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

First Pull Request #3

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

Conversation

KarinaAndersen1
Copy link

  • Minor changes to train_network (added dtype in hparams)
  • Added PINN_IV to network_configs
  • Accounted for devices with no GPU in utils

@MartinAstro
Copy link
Owner

Hey @KarinaAndersen1, thanks for putting this together. A few quick comments:

  • Would it be possible to remove the PINN-IV configuration from the config file? PINN I - III are fundamental changes/improvements to the model defined in a few papaers (architecture, feature engineering, data preprocessing, etc) and it looks like your proposed PINN IV is mostly experimental changes (different # of layers, nodes)l. Those are typically best kept in the train_network.py file itself and left uncommitted.
  • I like the LossComponentCallback you wrote, but in it's current form it appears to only print the RMS components, not actually the loss components (which can account for percent error as well). Could you update the callback to have a more representative name, or update it to log the full loss components?
  • In general, its probably best to leave the example scripts untouched unless a fundamentally new feature is being introduced into the repo. It looks like your changes are just (re)saving the history of the model and adding a plotting script. I'd recommend using the history as saved in the dataframe object which holds the hparams, and also moving the plotting script into the proper Scripts/ directory.

Would you mind fixing some of these issues in this PR or alternatively submitting multiple PRs with more compartmentalized changes? If the latter, I'll close this PR and review the others accordingly. Thanks!

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