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

support more schism variables #37

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

support more schism variables #37

wants to merge 4 commits into from

Conversation

wzhengui
Copy link
Collaborator

@wzhengui wzhengui commented Mar 3, 2022

Hi All:

This PR includes the following changes.

1). improve the Thalassa UI by adding necessary widgets and removing unnecessary widgets
2). improve the Thalassa structure, which I thinks makes it more organized, more extendable to include other models. To add another model, one just needs: a). add datasets read method (utils.py), and b). add the corresponding plot method in "api.py"
3). resolve the loading issue for large-size file
4). support transformation of different projection
5). support to show more SCHISM variables (2D, 3D, and 4D variables).
6). support native SCHISM outputs (schout_*.nc). No need for pro-processing
7). Support triangle and quadrilaterals (quads are split into two triangles).

Copy link
Collaborator

@saeed-moghimi-noaa saeed-moghimi-noaa left a comment

Choose a reason for hiding this comment

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

I take a very fast look. I leave it to @brey and @pmav99 for final review.

Copy link
Collaborator

@pmav99 pmav99 left a comment

Choose a reason for hiding this comment

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

  1. If you choose a netcdf with an unsupported format, there should be some indication on the UI that this specific file cannot be rendered and that you should choose a different one.
  2. It is no longer possible to display the maximum elevation of the whole timeseries. I think that this is an import feature that needs to be addressed.
  3. It should not be too hard to infer the actual "format" of the netcdf file. If that is the case, the UI can be simplified by removing the format combobox.
  4. The layer combobox is not relevant for all the netcdf's variables. If the implementation is not too difficult, it would be nice if it was active only for the ones that need it and deactivated for the rest.

thalassa/api.py Outdated
title=f"SCHISM Forecast: {self.variable}",
colorbar=True,
clabel="meters",
cmap="jet",
Copy link
Collaborator

@pmav99 pmav99 Apr 1, 2022

Choose a reason for hiding this comment

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

Using this colormap throws an exception:

# ...
  File "/home/panos/.conda/envs/thalassa/lib/python3.8/site-packages/holoviews/plotting/util.py", line 912, in process_cmap
    raise ValueError("Supplied cmap %s not found among %s colormaps." %
ValueError: Supplied cmap jet not found among matplotlib, bokeh, or colorcet colormaps.

We should either add matplotlib to the dependencies or use a different colormap. I wouldn't add an extra dependency just for a colormap.

Copy link
Collaborator Author

@wzhengui wzhengui Apr 1, 2022

Choose a reason for hiding this comment

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

Hi @pmav99 : Thank you for reviewing the PR. I think these are good suggestions. I will try to address them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @pmav99 and @brey : Thank Panos' useful suggestions again. I am sorry that I didn't have a chance talking to George this morning, as I was working on Thalassa. I just finished the revision except point 4. I tried to dynamically update the interface according to different variable picked by users, but it turns out to be very tricky. I will keep this on my mind, and see whether this is a good solution.

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