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

GraysHarbor and NY tidal examples #1

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

Conversation

YuchengZhang-hub
Copy link

No description provided.

@mandli
Copy link
Member

mandli commented Nov 2, 2021

@YuchengZhang-hub can you remove the output from the jupyter notebooks? Also it looks like this does not include what @rjleveque did for Grays Harbor. @rjleveque do you want to include that as well?

@YuchengZhang-hub
Copy link
Author

Sure, do you want me to create .html files for the jupyter notebooks that include outputs just like what Dr. Leveque did?

@mandli
Copy link
Member

mandli commented Nov 2, 2021

No, just make the notebooks not contain the output.

@YuchengZhang-hub
Copy link
Author

I just cleared all outputs from the notebooks.

@mandli
Copy link
Member

mandli commented Nov 8, 2021

It looks like there are still a number of figures (png files) in the PR. Are those from output?

@YuchengZhang-hub
Copy link
Author

The figures are used to explain things (for example, I used them to show the physical domain and gauge locations) in jupyter notebook, they are generated from make .plots and not from jupyter notebooks itself. They could be deleted if you want.

@mandli
Copy link
Member

mandli commented Nov 10, 2021

If they can be generated by running the code then they should definitely be removed.

@mandli
Copy link
Member

mandli commented Nov 10, 2021

A few additional changes:

  1. /GraysHarborBC/comparison - remove all the data that's here. Further I would suggest changing this from a notebook to a Python script that plots the results and put the markdown into the comments.
  2. GraysHarborBC - I feel like this directory structure is a bit more complex than it needs to be. I would imagine it should be possible to have the forced surface tide and the BC tide approach side by side in the same directory and then separate out the king tide case from the sine forced case?
  3. There is a lot of copies of nearly the same code, mostly bc2amr.f90 mostly.

@rjleveque
Copy link
Member

@YuchengZhang-hub: Thanks for working on this, I'm interested to see the comparisons of the approaches!

I just tried running the code in GraysHarborBC/RiemannGraysHarbor/kingtide2015 and it seems that height.txt is missing.

Following up on @mandli's comment regarding repeated code, you also have two different directories GraysHarborBC/RiemannGraysHarbor/topo and GraysHarborBC/SimpleGraysHarbor/topo that both have the same code for generating large topo files, which should be identical. So it might be better to have a single GraysHarborBC/topo directory for the Grays Harbor topo.

@YuchengZhang-hub
Copy link
Author

Hello Dr. @rjleveque , the height.txt could be generated from the Jupyter notebook and it should be moved to _output subdirectory.

Following Dr. @mandli 's comment, I'm working on simplifying the directories, I should be able to have a new commit ready tomorrow.

@rjleveque
Copy link
Member

Thanks, I got the example to run fine.

Another thing I noticed is that you read in the full height.txt file every time bc2amr is called, which happens several times each time step (once on each grid patch). I think it would be better to read this in once in setprob.f90, into an array that is then available in bc2amr via a use statement.

@YuchengZhang-hub
Copy link
Author

Great idea, thanks. I'm going to implement it now.

I just made a lot of changes to the structures of my directories, I also made height.txt take into account of time shift and scaling so it doesn't need to be implemented in bc2amr.f90.

I also included your ocean forcing code, hope that's OK or I could delete it.

@mandli
Copy link
Member

mandli commented Nov 13, 2021

Which ocean forcing code?

@YuchengZhang-hub
Copy link
Author

Dr. Leveque's code, the forced surface tide (https://github.com/rjleveque/geoclaw_tides/tree/main/GraysHarbor)

@mandli
Copy link
Member

mandli commented Nov 15, 2021

@rjleveque are you ok with us moving your other code into this one?

@rjleveque
Copy link
Member

Yes, I think it's a good idea to include that code here as well. I'll try to go through these examples again in more detail soon.

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