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

updated tidetools.py and usage examples notebook #5

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

socoyjonathan
Copy link
Contributor

No description provided.

@rjleveque
Copy link
Member

I got the Tidetools_Module_Examples.ipynb notebook to run after adjusting the paths a bit (see below), and I think it reads better now, thanks for the adding the new discussion.

We need to decide where pytides and tidetools.py are going to reside, and I realize my comments in some of the code in #4 was confusing, when I said we'd like to be able to do:

   from clawpack.pytides.tide import Tide

which would try to import directly from $CLAW/pytides.

But then I was thinking we would also have clawpack.pytides as a submodule in this$CLAW/tidal-examples repository, in which case we presumably want to import as:

   from clawpack.tidal-examples.pytides.tide import Tide

Maybe it's cleaner to not include it as a submodule and instead just import clawpack.pytides. But then the user has to clone that respository separately and check out the correct geoclaw branch to get our fixes to the original pytides, and I think we also need to add pytides to the setup.py in Clawpack so that the import statement works.

Also, the current import statement

    from clawpack.geoclaw import tidetools

tries to import tidetools.py from $CLAW/geoclaw/src/python/geoclaw/ and won't be using the one that's in this repository at all. Maybe eventually we want to move it there, but for testing out these notebooks we will have to stick to a path that leads to the local version of the file.

Perhaps we should chat about this with @mandli before deciding how best to organize.

@socoyjonathan
Copy link
Contributor Author

@rjleveque: Thank you for the updates! I agree with your concerns on the paths issues and I appreciate the clarifications. Please let me know how to best proceed after further discussion with @mandli!

@mandli
Copy link
Member

mandli commented Apr 29, 2022

I am in agreement that we may want to think about where to put the pytides submodule more permanently rather than simply have it contained in the apps repository. As @rjleveque mentioned there is a way to do this without users having to worry about checking that repository out, we just need to modify the setup.py.

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