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

render rollout merge #87

Merged
merged 26 commits into from
Oct 23, 2024
Merged

render rollout merge #87

merged 26 commits into from
Oct 23, 2024

Conversation

Naveen-Raj-M
Copy link

@Naveen-Raj-M Naveen-Raj-M commented Sep 9, 2024

Objective:
This RFP proposes to integrate the execution of rollout prediction and rendering function. It should also provide an option to disable the automatic rendering

Current System:
Currently, the user has to execute rollout prediction and rendering through separate commands.

Proposed Changes:

  1. In the gns/config.yaml file, add default configurations for rendering. By default, the render is set to true in the rendering configuration
  2. In the gns/train.py script, within the predict method, after generating the example rollout file, check if config.rendering.render is set to true. If so, invoke the rendering method to handle the rendering process.
  3. The rendering function takes the filename and directory as arguments and calls the render class from the render_rollout module.
  4. Based on the configuration, either a GIF animation is generated using the render_gif_animation method or VTK output is produced using the write_vtk method from the render_rollout module
  5. README.md is updated to describe this feature

Expected Challenges:

  1. If multiple rollouts are predicted with a single input file, this feature will render for each predicted example. But, this can be overcome by setting the rendering.render option to be False and render separately with the specific rollout file.

Testing Plan:
The following steps are used to test the functioning of rendering function in gns/train.py script,

  1. Fixtures are set up to generate temporary directories and dummy .pkl data for testing purposes.
  2. Configuration Fixtures (cfg_gif and cfg_vtk) provide configuration objects for rendering GIFs or VTK output, respectively, using the DictConfig from omegaconf.
  3. Two test functions (test_rendering_gif and test_rendering_vtk) are used to test if the rendering logic for GIFs and VTKs works correctly.
  4. The rendering function (imported from gns.train) is called with the corresponding configuration, and the tests assert the creation of output files (GIF for the GIF test, and two VTK directories for the VTK test).
  5. The temporary directories and files are cleaned up after the tests.

Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

  • You need to update readme.
  • Write a test to see if rollouts are predicted correctly
  • Don't you need to update or define the hydra configs?

config.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

Document all your functions properly using docstrings -- Google style

README.md Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
config.yaml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
gns/args.py Outdated Show resolved Hide resolved
temp/test_vtk.py Outdated Show resolved Hide resolved
temp/test_vtk.py Outdated Show resolved Hide resolved
temp/test_vtk.py Outdated Show resolved Hide resolved
temp/test_vtk.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

Refactor the code and be consistent, improve readability.

README.md Outdated Show resolved Hide resolved
test/test_vtk.py Outdated Show resolved Hide resolved
gns/train.py Show resolved Hide resolved
gns/train.py Show resolved Hide resolved
test/test_vtk.py Outdated Show resolved Hide resolved
test/test_vtk.py Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/test_vtk.py Outdated Show resolved Hide resolved
test/test_vtk.py Outdated Show resolved Hide resolved
test/test_vtk.py Show resolved Hide resolved
test/test_vtk.py Outdated Show resolved Hide resolved
test/test_vtk.py Outdated Show resolved Hide resolved
utils/count_n_files.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kks32 kks32 left a comment

Choose a reason for hiding this comment

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

Fix the black failures, then we should be good to merge

@kks32 kks32 merged commit 7190e4a into geoelements:v2 Oct 23, 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