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

Add mermaidjs based code diagram #9

Closed
wants to merge 2 commits into from

Conversation

leifdenby
Copy link
Member

@leifdenby leifdenby commented Feb 12, 2024

To aid discussion around refactoring the code and allow for easy modification of the code diagram in future I have created a mermaidjs based diagram in the README in place of the graphic that was there previously. Please have a look @joeloskarsson if you think this covers what the old diagram did.

Here's a direct link in the README: https://github.com/leifdenby/neural-lam/tree/feat/readme-code-diagram?tab=readme-ov-file#pre-processing, and a screenshot from vscode:

image

Leif Denby added 2 commits February 12, 2024 12:03
To aid discussion around refactoring the code and allow for easy modification
of the code diagram in future introduce a [mermaidjs](https://mermaid.js.org/)
based diagram in the README.
@leifdenby
Copy link
Member Author

@joeloskarsson I updated the diagram because I realised that I had slightly misunderstood the shapes of what is in the .npy-files. I made the following notebook which plots each of the inputs you use, just for myself to learn what the shapes of what everything is: https://github.com/leifdenby/neural-lam/blob/examining-inputs/notebooks/examining_inputs.ipynb

Copy link
Collaborator

@joeloskarsson joeloskarsson left a comment

Choose a reason for hiding this comment

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

I think this is great, and much more maintainable than the diagram I had made before. Just fix the missing file and (optionally) see if you can re-style the notes, and I'll merge.

As we refactor I hope to abstract some of these things away, but having this diagram also to change when we are doing such refactoring is very valuable.


subgraph parameter_weights
direction LR
parameter_mean.pt\nparameter_std.pt\ndiff_mean.pt\ndiff_std.pt\nflux_stats.pt:::pt_file
Copy link
Collaborator

Choose a reason for hiding this comment

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

The file parameter_weights.npy is missing here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I realise that this is a pretty bad name for this specific file, but that's for another day 😅

source_xy_coordinates
surface_geopotential
border_mask
note("stored in `data/{dataset_name}/static/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How tricky would it be to make these notes look more different than the files? It would be nice just to avoid confusion. But if it is not trivial to change the styling a bit it doesn't matter.

@joeloskarsson
Copy link
Collaborator

Should we fix and merge this, or is this diagram obsolete as we are moving to other ways of handling data loading? I just don't see a need for this to sit here as an open PR.

@leifdenby
Copy link
Member Author

I think this is obsolete now :)

@leifdenby leifdenby closed this Aug 5, 2024
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.

2 participants