Skip to content

Clean up - #123

Open
charlie-becker wants to merge 7 commits into
mainfrom
becker
Open

Clean up#123
charlie-becker wants to merge 7 commits into
mainfrom
becker

Conversation

@charlie-becker

Copy link
Copy Markdown
Collaborator
  • Add doc strings
  • fix minor bud in random sampling
  • add post process script to map pot leaks to equipment ID

Successfully tested code for generating / sampling LES data and training of all model types. Still need to add a evaluation notebook that come shortly.

@charlie-becker

Copy link
Copy Markdown
Collaborator Author

Added a couple of eval notebooks.

@ThomasMGeo

Copy link
Copy Markdown
Contributor

Don't know how I missed this, this looks great to me.

@jsauer-NCAR jsauer-NCAR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Docstrings and cleanup generally improve things thanks.

For the change in data.py (lines 595-596) away from open_mfdataset to an inline concatenation (along sample dimension) of separately opened, but combined eagerly loaded files (in lines 598-599), does this actually improve performance?

I'd like to suggest someone go through and make sure we do not have any hard coded paths anywhere in the repository that are specific to our work in the NCAR HPC environment. Since the purpose of these latest revisions are to prepare for an open release of this repository and code base it should probably not contain any of our own specific paths.

@charlie-becker

Copy link
Copy Markdown
Collaborator Author

For the change in data.py (lines 595-596) away from open_mfdataset to an inline concatenation (along sample dimension) of separately opened, but combined eagerly loaded files (in lines 598-599), does this actually improve performance?

I have noticed an improvement using this method for other projects, but I'm unsure if it is universally better. I think it often depends on chunking, dimension sizes (i.e. if a concatenation dimension already exists or not, etc.), or other factors.

I'd like to suggest someone go through and make sure we do not have any hard coded paths anywhere in the repository that are specific to our work in the NCAR HPC environment

I can go through and verify that all scripts and modules are free of GLADE paths. But what about evaluation notebooks? It is unlikely we'll be able put any meaningful amount of data in the actual repository for analysis. Some potential options:

  • Remove the analysis notebooks all together
  • Remove the paths of the hard coded data but keep the rendered HTML notebook output as examples
  • Point to downloading data from zenodo (or other data repository) and perform analysis notebooks on those.
  • Other?

@jsauer-NCAR

Copy link
Copy Markdown
Collaborator

I feel like option 3 above is most ideal, but perhaps a heavier or longer-time-requirement lift. Option 2 seems the most effective/economical in terms of effort and time in the short term. @charlie-becker Do you think we could put in placeholders as comment/explicit-guidance lines like "blah=INSERT_PATH_TO_YOUR_DATA_FILE" kind of thing wherever appropriate?

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