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

🐛[BUG][Feature Request]: Perturbing channels that are not included in earth2mip/_channel_stds.py #186

Open
ankurmahesh opened this issue Apr 9, 2024 · 1 comment
Labels
0 - Backlog In queue waiting for assignment bug Something isn't working

Comments

@ankurmahesh
Copy link
Contributor

ankurmahesh commented Apr 9, 2024

Version

source - main

On which installation method(s) does this occur?

Source

Describe the issue

When the channels are perturbed, they are multiplied by scale.

https://github.com/NVIDIA/earth2mip/blob/main/earth2mip/inference_ensemble.py#L246-L253

However, scale is determined from the values in this file, not from the scales stored in the model Inference

https://github.com/NVIDIA/earth2mip/blob/main/earth2mip/_channel_stds.py

I use a dataset that uses q, not r. Therefore, by the logic specified in code segment above, all of the q variable perturbations are set to 0 because q is not in the _channels_stds.py file. This wasn't my intended behavior: I intended to perturb q.

Three possible solutions:

  1. We delete the _channel_stds.py and instead use the model.scale in the perturbation. The perturb method has access to the model. Inference models have a scale attribute.
  2. If we keep the current logic, maybe we could add a logger warning that says "X channel is not perturbed". As more models get trained, I think it's likely that more variables get added (e.g. more vertical levels). Since the scales are being drawn from a separate file, not from the scales.npy file in the model package, I think an alert could be useful.
  3. I could just add the q scales to _channel_stds.py. But I would still recommend (2) above as well.

Environment details

I am running from source.  I currently see 86b11fe.
@ankurmahesh ankurmahesh added ? - Needs Triage Need team to review and classify bug Something isn't working labels Apr 9, 2024
@ankurmahesh ankurmahesh changed the title 🐛[BUG]: Perturbing channels that are not included in earth2mip/_channel_stds.py 🐛[BUG][Feature Request]: Perturbing channels that are not included in earth2mip/_channel_stds.py Apr 9, 2024
@nbren12
Copy link
Collaborator

nbren12 commented Apr 13, 2024

  1. We delete the _channel_stds.py and instead use the model.scale in the perturbation.

Not all models have .scale. It is not part of the TimeLoop interface defined here:

class TimeLoop(Protocol):
"""Abstract protocol that a custom time loop must follow

This is why we took steps to decouple the initialization from the model. There are also many potential ways to scale perturbations e.g. scale by climatological variance, and these should be applicable across models (even ones which don't have .scale).

Overall these perturbation methods could use an overhaul and refactor to a more modular design. e.g. one class per method. My solution would be to ask ChatGPT "please refactor this if-statement with many clauses to one class per if statement, this class should take a dictionary of channel scales as an argument to its constructor".

For now, (2) and (3) above would be nice.

@nbren12 nbren12 added 0 - Backlog In queue waiting for assignment and removed ? - Needs Triage Need team to review and classify labels Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Backlog In queue waiting for assignment bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants