Skip to content

Switch to using np.arctan2 for Probabilistic Advection#2

Open
KristianHMoller wants to merge 2 commits into
dmidk:manage-depsfrom
KristianHMoller:bugfix/arctan
Open

Switch to using np.arctan2 for Probabilistic Advection#2
KristianHMoller wants to merge 2 commits into
dmidk:manage-depsfrom
KristianHMoller:bugfix/arctan

Conversation

@KristianHMoller

Copy link
Copy Markdown

It was noted, that even with the alpha and beta noise parameters both set to zero, running probabilistic_advection in ensemble mode (e.g. by not specifically requesting ens_members=1) yielded results different from those obtained in deterministic mode.

The source of the discrepancy was tracked to the calculation of pert_motion_field in the script Models/ProbabilisticAdvection.py. The original code:

        pert_motion_field[0, :, :] = pert_abs * np.cos(np.arctan(y / x) + beta_noise)
        pert_motion_field[1, :, :] = pert_abs * np.sin(np.arctan(y / x) + beta_noise)

np.arctan does not consider the signs of the input values and additionally, the implementation will fail for x=0. https://numpy.org/doc/stable/reference/generated/numpy.arctan.html

By replacing np.arctan with np.arctan2, these issues are avoided. https://numpy.org/doc/stable/reference/generated/numpy.arctan2.html

@KristianHMoller

Copy link
Copy Markdown
Author

The update to using np.arctan2 as opposed to np.arctan leads to agreement between deterministic and ensemble mode when specifying the noise (alpha and beta) to be zero:

nowcast_3x2_h5

@KristianHMoller

Copy link
Copy Markdown
Author

@khintz do you have permission to add reviewers?
Perhaps yourself or @elbdmi ?

@khintz

khintz commented Jun 29, 2026

Copy link
Copy Markdown

You should have permissions to request reviewers now.

@KristianHMoller KristianHMoller requested review from elbdmi and khintz June 29, 2026 09:16
@khintz

khintz commented Jun 29, 2026

Copy link
Copy Markdown

That backfired 😉 ... Should this go into EnergyWeatherAI:main instead or do you prefer a different PR to that, to get this in asap?

@elbdmi elbdmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This change is resonable for me. The only this I would like that doesn't have to do with this pull request but it is a general one is to create a CHANGELOG and also add your change to the changelog as well. It would be nice also if you could add a comment in the code in order to know why we have arctan2 and not arctan

@KristianHMoller

Copy link
Copy Markdown
Author

That backfired 😉 ... Should this go into EnergyWeatherAI:main instead or do you prefer a different PR to that, to get this in asap?

I certainly think, that we should also have a PR to EnergyWeatherAI:main as well. I will do that after the meeting. But I do not know how much that is maintained. It is not urgent to get into our code before we start using ensembles with the updated version of sunflow. The version currently running operationally does suffer from these artifacts.

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