-
Notifications
You must be signed in to change notification settings - Fork 78
feat: Reconstruction plots callback #782
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
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
…i-core into feature/reconstruction-plots
for more information, see https://pre-commit.ci
…i-core into feature/reconstruction-plots
for more information, see https://pre-commit.ci
…i-core into feature/reconstruction-plots
for more information, see https://pre-commit.ci
…i-core into feature/reconstruction-plots
| ymin, ymax = max(lat.min(), -np.pi / 2), min(lat.max(), np.pi / 2) | ||
| ax.set_xlim((xmin - 0.1, xmax + 0.1)) | ||
| ax.set_ylim((ymin - 0.1, ymax + 0.1)) | ||
| if transform is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of this seems to be could be better abstracted and integrated into training/src/anemoi/training/diagnostics/maps.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure which parts you refer too here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replied on the other PR, let me know if it's unclear
| cmap: Colormap | None = None, | ||
| error_cmap: Colormap | None = None, | ||
| ) -> None: | ||
| """Plot one variable's input, reconstruction, and error map on a flat lat/lon grid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it fair to say this is a special case for the plot_predicted_multilevel_flat_sample where we don't have a target but rather the input and the pred? so it would like the plotting a diagnostic variable but where target is input?
Thinking if would dramatically reduce the amount of code if we could generalise the plot_predicted_multilevel_flat_sample to work with that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but the function was already quite complex and had many different columns.
As it is a different layout, I preferred to just create a new one rather than extending this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue that I see at the moment is that projection and transform are specific and will just work if one has cartopy installed. So I would like to find a way we can define this and reuse it for the other plots so that we reuse the functionality we currently have but with the option of customising the projections and the transform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't want to make it completely configurable from the config I think a clearer design would be something like
@dataclass(frozen=True)
class MapContext:
"""Encapsulates map projection, data CRS, and extent handling."""
projection: object | None
data_crs: object | None
pad: float = 0.1
@classmethod
def from_latlons(
cls,
latlons: np.ndarray,
*,
datashader: bool = False,
pad: float = 0.1,
) -> MapContext:
try:
import cartopy.crs as ccrs
except ModuleNotFoundError:
return cls(projection=None, data_crs=None, pad=pad)
if datashader:
return cls(projection=None, data_crs=None, pad=pad)
projection = lambert_conformal_from_latlon_points(latlons)
data_crs = ccrs.PlateCarree()
return cls(projection=projection, data_crs=data_crs, pad=pad)
def set_extent(
self,
ax,
lon: np.ndarray,
lat: np.ndarray,
) -> None:
"""Set axis extent consistently for projected or flat axes."""
if self.data_crs is not None and hasattr(ax, "set_extent"):
ax.set_extent(
[
lon.min() - self.pad,
lon.max() + self.pad,
lat.min() - self.pad,
lat.max() + self.pad,
],
crs=self.data_crs,
)
else:
xmin, xmax = max(lon.min(), -np.pi), min(lon.max(), np.pi)
ymin, ymax = max(lat.min(), -np.pi / 2), min(lat.max(), np.pi / 2)
ax.set_xlim(xmin - self.pad, xmax + self.pad)
ax.set_ylim(ymin - self.pad, ymax + self.pad)
and we then could pass this to the plots like
from map_context import MapContext
map_ctx = MapContext.from_latlons(
latlons,
datashader=datashader,
)
fig, ax = plt.subplots(
n_vars,
n_plots_per_sample,
figsize=(n_plots_per_sample * 4, n_vars * 3),
layout="constrained",
subplot_kw={"projection": map_ctx.projection},
)
so the MapContext will handle all the logic and code related to cartopy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also some of those settings could be defined at the level of callback ie if you have a specific AutoencoderPlotSample then there we can define those and try to reuse the plot_predicted_multilevel_flat_sample
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the topic of creating a new function vs using the existing one, from what I see it's mostly the definition of the projection and the fact that for the autoencoder you just one 3 subplots, vs the forecasting one includes 6 subplots right? So I think this could be configured to define this at the callback level and then reuse the functions that we have.
I understand having the functions separated might make it easier to understand but bugs and features in the future would then need to be copied across both of them, making dev workflow more cumbersome so I would prefer we homogenise it.
| data, output_tensor = self.process(pl_module, outputs, batch, output_times) | ||
|
|
||
| # Compute focus mask | ||
| focus_mask = self.get_focus_mask(pl_module) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These needs to be repeated for all callbacks - we should have a function to clip the data as part of the basePlot and then reuse it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? various callbacks can have different focus_areas, you won't know until you reach every callback plot, no?
anaprietonem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Alberto, I had a look I think the focus_area parameter is a nice feature!
I left some comments for clarification and mostly I think the reconstruction plot callback and how the LAEA projection is handled can be abstracted a bit more so the amount of code changes can be reduced. Let me know if anything is not clear
|
Hey @anaprietonem thanks for reviewing, addressed majority of the requested changes, could you clarify some things in the above comments? |
…i-core into feature/reconstruction-plots
|
Hi Alberto, first of all, thanks for the PR. I think this focus_area functionality can be very useful for many users. Below are a few thoughts and suggestions.
These comments are mainly intended to raise discussion rather than to suggest strict changes. Happy to hear your thoughts and suggestions. Thanks again for your contribution! |
|
Hey @JPXKQX , thanks for taking a look¦
Will ask your review in the spawened PRs ;) |
Description
Introducing focus area to plot only specific regions.
Introducing reconstruction plots to compare input and output and their difference.
As a contributor to the Anemoi framework, please ensure that your changes include unit tests, updates to any affected dependencies and documentation, and have been tested in a parallel setting (i.e., with multiple GPUs). As a reviewer, you are also responsible for verifying these aspects and requesting changes if they are not adequately addressed. For guidelines about those please refer to https://anemoi.readthedocs.io/en/latest/
By opening this pull request, I affirm that all authors agree to the Contributor License Agreement.
📚 Documentation preview 📚: https://anemoi-training--782.org.readthedocs.build/en/782/
📚 Documentation preview 📚: https://anemoi-graphs--782.org.readthedocs.build/en/782/
📚 Documentation preview 📚: https://anemoi-models--782.org.readthedocs.build/en/782/