-
Notifications
You must be signed in to change notification settings - Fork 8
Fix NN search #644
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
base: main
Are you sure you want to change the base?
Fix NN search #644
Conversation
| ncount = 0 | ||
| DO kk = 1, mland | ||
| distance = 5300.0 ! initialise, units are degrees | ||
| distance = 144.0 ! initialise, units are degrees^2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure on the choice of 144. here - this need to be larger (in degrees^2) than the expected largest gridcell. 144 here corresponds to a 12 degree-squared cell (so 30x15 cells in a global simulation) - so coarse but as coarse as it could be
I would have thought that 64800 would have been the safest option for this initialisation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, my only reason for choosing this value is that it's what was chosen in the CABLE-POP_TRENDY branch- effectively saying if you can't find any valid source data within a 12 degree circle, crash out. Is this something desired? Or should it be if it finds a valid neighbour anywhere on the globe, then that's ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay - that makes some sense. In effect it's trying to say if there isn't a valid lat/lon within the 12-degree circle then you need to fix your gridinfo - we could encouter this type of thing in high resolution simulations where we get some new land appearing (think Pacific/southern ocean islands).
Probably just needs a comment to explain the choice of 144
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment has been added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I don't know what the best behaviour should be here. I'll try to raise this with people using the main branch of CABLE for offline simulations and get their opinions before merging this.
har917
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 comments:
- choice of default initialisation - I think the 144 could be increased without any impact and it would then be robust to very coarse resolution simulations.
- I think we need a comment in here somewhere stating that this isn't truly nearest neighbour (in terms of distance) as would be inferred from the routine name. We're actually finding the lat/lon pair that minimises the quadratic-degree metric. This is not the same as distance (as would be measured by a ruler) because of the sin(latitude)-dependence on arc length.
I've added a comment above each search saying: "This search is not a nearest neighbour in the true sense, due to the latitude dependence on arc length. It is only a nearest neighbour in the latitude/longitude coordinate space." |
…initial distance^2
CABLE
Description
The nearest neighbour searches in
src/offline/cable_parameters.F90were wrong around the antimeridian- did not account for the wrap around at -180/180. The use of SQRT in computing the distance was also unnecessary, assuming you also set your upper bound for the distance appropriately. There are two instances of this search, here and here. The first instance sets a maximum distance in degrees to be 5300 (?!), while the second 300. Neither of these make sense as maximum distances.CABLE_POP_TRENDYsets a maximum of distance of 12 degrees before the search fails, which seems more reasonable (although happy to adjust this value).Fixes #643
Type of change
Checklist
Testing
📚 Documentation preview 📚: https://cable--644.org.readthedocs.build/en/644/