Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

some miscellaneous updates #225

Merged
merged 6 commits into from
May 31, 2021
Merged

some miscellaneous updates #225

merged 6 commits into from
May 31, 2021

Conversation

WPringle
Copy link
Collaborator

@WPringle WPringle commented May 13, 2021

  • change order in cmocean for applying the layer interpolation to the end (seems better to me)
  • change order of applying cmocean in plotter to the end
  • adding radius_separated_points function that is to be used before m_quiver function to ensure that you have get equal spatial distribution of arrows
  • change to tidal_data_to_ob function so that it keeps all tidal constituents for the boundary but sets to zero those which are not in the database to make it easier for user to set themselves later if desired.
  • correcting m_grid call in msh.plot() to use the background color (backcolor option)

…he end (seems better to me)

- change order of applying `cmocean` in `plotter` to the end
- adding `radius_separated_points` function that is to be used before m_quiver function to ensure that you have get equal spatial distribution of arrows
- change to `tidal_data_to_ob` function so that it keeps all tidal constituents for the boundary but sets to zero those which are not in the database to make it easier for user to set themselves later if desired.
% so that they are at least separated by a distance of
% user-defined 'radius'

% computing rangesearch
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ourRangeSearch to avoid toolbox dependency if possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ourRangeSearch doesn't seem to work for multiple points.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you call it in a loop over all the query points.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see this line there's some unfortunately hardcoded parameters.
[idx,~] = frsearch(anno, testset, r, 1e3,0,1);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok updating that now..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

latest commit addresses this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but note for test I was using it took 3 sec for rangesearch but 50 sec for ourRangeSearch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, it's not vectorized. I guess you use this for quiver plotting. What you could do is check if the rangesearch exists if they have the toolbox and then default to this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it does do that

keep(j) = false;
continue
disp(['No tidal data in file for constituent ' const{j}])
obj.f15.opealpha(j).name = const{j};
Copy link
Collaborator

@krober10nd krober10nd May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What’s the use case for this? I didn’t understand your explanation in the commit

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use case I was doing is the following:
const = ["Z0" "major8"];

The Z0 is the zero frequency constituent that is actually valid in the constituent list but TPXO8 provides no data. So before I would just have this deleted from the boundary condition list. However, I was using Z0 to impose a sea level rise offset.

So I was using Make_f15 to populate Z0 with "zeros" as a dummy into the boundary constituent list then overwrite it later with the desired amplitude. An additional thing I was thinking to add was in writefort15.m that it would only write it out if it wasn't all zeros to avoid cluttering of fort.15 but I think this could be considered a fairly minor concern unless you have many constituents.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so in the case you're applying a SLR offset using this tidal constituent, you also add a geoid offset to the NA file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I did not but I suppose you could (should?) do that as initial condition yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using small domain and just spun it up slowly to that state.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea I think you should specify an initial condition that matches the elevation of the boundary condition otherwise you'll create a large gradient near the boundary in free surface elevation.

but this for some more advanced users anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah but since you spin it up from zero it works. And also even if elevation is below the new sea level with SLR it may not get flooded due to connectivity so that's why I just did the boundary condition

…oints could end up going slightly below this causing NaNs in the projection
…ints are not pushed outside and become NaNs (was limited to radius of 178 deg but made sure can go up to full 180 deg)
@krober10nd
Copy link
Collaborator

change order in cmocean for applying the layer interpolation to the end (seems better to me)

What does this mean?

@WPringle
Copy link
Collaborator Author

change order in cmocean for applying the layer interpolation to the end (seems better to me)

What does this mean?

So in cmocean it was applying the interpolation of the continuous colormap to discrete levels somewhere in the middle of the code, however I changed it to the end.

The reason being was that for the colormap with pivots the interpolation to the discrete levels before the pivot application was causing problems because the pivot will not be precisely applied once the colormap has been reduced to a few discrete levels. I found better performance with it at the end with pivots e.g., when plotting the bathymetry 'b'.

@krober10nd
Copy link
Collaborator

yea, there's often random issues with plotting some quantities that require you tweak the colormap option. I guess this helps reduce the occurrence of those issues.

…to ourRangeSearch in case rangesearch does not exist in radius_separated_points function
@krober10nd
Copy link
Collaborator

changelog?

@WPringle
Copy link
Collaborator Author

changelog?

look ok?

@krober10nd
Copy link
Collaborator

Looks great to me.

@WPringle WPringle merged commit 543ffb6 into Projection May 31, 2021
@WPringle WPringle deleted the misc_updates branch May 31, 2021 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants