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

Update to LCS algorithms #479

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

johannesro
Copy link
Collaborator

  • bugfixes
  • return eigenvector and eigenvalue to calculate strainlines
  • distinguish between LCS and FLTE
  • add example script for strainline calculation


# Make points, evenly distributed
deltax = np.sqrt(area/number)
lonpoints = np.array([])
area = abs(area) / 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems generally good, but 4 tests are failing, presumably because this line has been moved, and thus affecting the area and giving the wrong number of elements. Is this a mistake?

@@ -2043,7 +2043,7 @@ def set_fallback_values(self, refresh=False):

def run(self, time_step=None, steps=None, time_step_output=None,
duration=None, end_time=None, outfile=None, export_variables=None,
export_buffer_length=100, stop_on_error=False):
export_buffer_length=100, stop_on_error=False, move_from_land=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

move_from_land seems to be a duplicate of existing config option seed:ocean_only? Thus I believe move_from_land should be removed, and config should be used instead, for consistency.

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 don't remember exactly why I did this, but it was necessary to get it work, and it is different from seeding

Copy link
Collaborator

@knutfrode knutfrode Nov 24, 2021

Choose a reason for hiding this comment

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

We should avoid more input parameters to methods like run, and instead control such things via the config-mechanism. There are several reasons for this, one being that web interfaces like Drifty are based generally on the config-mechanism, and there we want to avoid any method-specific tweaks that must be hardcoded and kept in sync for the future.

vilhelmfagerstrom and others added 5 commits November 18, 2021 17:56
Include deativation of larvae due to spawning, settling and death
('land_binary_mask' not in self.fallback_values) and \
('land_binary_mask' in self.required_variables):
self.timer_start('preparing main loop:moving elements to ocean')
self.elements_scheduled.lon, self.elements_scheduled.lat = \
Copy link
Collaborator

@knutfrode knutfrode Nov 24, 2021

Choose a reason for hiding this comment

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

It seems here that you have simply overwritten the config seed:ocean_only with the new move_from_land=True, and this is probably one (of several?) reason for the tests failing.
Anyway, if starting from a fresh, rebased fork, it should be quite straightforward to make a new PR with the same additions as in this PR. So two things are essential: rebasing against master, and running pytest before committing.

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.

3 participants