add additional column case in plot_training

[SavvasMel]

Description

Introduces a small "if statement" to cover channels with "." in the last parts of the string. This allow for:

Issue Number

Closes #2767

Is this PR a draft? Mark it as draft.

Checklist before asking for review

• I have performed a self-review of my code
• My changes comply with basic sanity checks:
  • I have fixed formatting issues with ./scripts/actions.sh lint
  • I have run unit tests with ./scripts/actions.sh unit-test
  • I have documented my code and I have updated the docstrings.
  • I have added unit tests, if relevant
• I have tried my changes with data and code:
  • I have run the integration tests with ./scripts/actions.sh integration-test
  • (bigger changes) I have run a full training and I have written in the comment the run_id(s): launch-slurm.py --time 60
  • (bigger changes and experiments) I have shared a hegdedoc in the github issue with all the configurations and runs for this experiments
• I have informed and aligned with people impacted by my change:
  • for config changes: the MatterMost channels and/or a design doc
  • for changes of dependencies: the MatterMost software development channel

FastEvaluation

• I have updated the public documentation if necessary

[clessig]

I don't have a strong opinion but wouldn't it be best to sanitize the channel names when the data is loaded and, e.g., replace "." with "_"?

[jpolz]

Looks good!

[SavvasMel]

I don't have a strong opinion but wouldn't it be best to sanitize the channel names when the data is loaded and, e.g., replace "." with "_"?

The data has been transferred to ml_training folder so someone with write/read access rights needs to this, and also the sanitation needs to take place also for all the v3 zarr files. It is not a challenge or anything per se to have a script to do this automatically, or something that an agent cannot do fast and clean, but I find it time consuming, even only the effort on pinging people just for a ".".

Also to be fair I do not really find self explained having 2.5 as 2_5. What do you think?

[clessig]

I don't have a strong opinion but wouldn't it be best to sanitize the channel names when the data is loaded and, e.g., replace "." with "_"?

The data has been transferred to ml_training folder so someone with write/read access rights needs to this, and also the sanitation needs to take place also for all the v3 zarr files. It is not a challenge or anything per se to have a script to do this automatically, or something that an agent cannot do fast and clean, but I find it time consuming, even only the effort on pinging people just for a ".".

Also to be fair I do not really find self explained having 2.5 as 2_5. What do you think?

No, I mean when we read in the training log files we should santize the column name to a valid format.

[SavvasMel]

I don't have a strong opinion but wouldn't it be best to sanitize the channel names when the data is loaded and, e.g., replace "." with "_"?

The data has been transferred to ml_training folder so someone with write/read access rights needs to this, and also the sanitation needs to take place also for all the v3 zarr files. It is not a challenge or anything per se to have a script to do this automatically, or something that an agent cannot do fast and clean, but I find it time consuming, even only the effort on pinging people just for a ".".
Also to be fair I do not really find self explained having 2.5 as 2_5. What do you think?

No, I mean when we read in the training log files we should santize the column name to a valid format.

I tried, if you sanitize the column names then the legends are affected necessarily:

Also you cannot avoid but to have something of the sort to correct the titles. I believe sanitizing is much more prune to bugs/errors in the future.

                            elif len(col_split) == 4:
                                if (
                                    col_split[1].lower() == stream_name.lower()
                                    and col_split[2].lower() == err.lower()
                                    and col_split[3] == channel_sanitized
                                ):
                                    data_cols += [col]
                                    title_col = (
                                        f"LossPhysical.{stream_name}.{err}.{channel_display}"                 <---------
                                        if title_col is None
                                        else title_col
                                    )
                            elif len(col_split) == 5:
                                if (
                                    col_split[1].lower() == stream_name.lower()
                                    and col_split[2].lower() == err.lower()
                                    and col_split[3] == channel_sanitized
                                    and int(col_split[4]) in forecast_steps
                                ):
                                    data_cols += [col]
                                    title_col = (
                                        f"LossPhysical.{stream_name}.{err}.{channel_display}"               <---------
                                        if title_col is None
                                        else title_col
                                    )

[clessig]

I don't have a strong opinion but wouldn't it be best to sanitize the channel names when the data is loaded and, e.g., replace "." with "_"?

The data has been transferred to ml_training folder so someone with write/read access rights needs to this, and also the sanitation needs to take place also for all the v3 zarr files. It is not a challenge or anything per se to have a script to do this automatically, or something that an agent cannot do fast and clean, but I find it time consuming, even only the effort on pinging people just for a ".".
Also to be fair I do not really find self explained having 2.5 as 2_5. What do you think?

No, I mean when we read in the training log files we should santize the column name to a valid format.

I tried, if you sanitize the column names then the legends are affected necessarily:

Also you cannot avoid but to have something of the sort to correct the titles. I believe sanitizing is much more prune to bugs/errors in the future.

                            elif len(col_split) == 4:
                                if (
                                    col_split[1].lower() == stream_name.lower()
                                    and col_split[2].lower() == err.lower()
                                    and col_split[3] == channel_sanitized
                                ):
                                    data_cols += [col]
                                    title_col = (
                                        f"LossPhysical.{stream_name}.{err}.{channel_display}"                 <---------
                                        if title_col is None
                                        else title_col
                                    )
                            elif len(col_split) == 5:
                                if (
                                    col_split[1].lower() == stream_name.lower()
                                    and col_split[2].lower() == err.lower()
                                    and col_split[3] == channel_sanitized
                                    and int(col_split[4]) in forecast_steps
                                ):
                                    data_cols += [col]
                                    title_col = (
                                        f"LossPhysical.{stream_name}.{err}.{channel_display}"               <---------
                                        if title_col is None
                                        else title_col
                                    )

But is it an issue to have ...thetao_2_5 instead of ...thetao_2.5 ? plot_train is an absolute expert system and the 3 people that would be affected would know, not?

[SavvasMel]

I don't have a strong opinion but wouldn't it be best to sanitize the channel names when the data is loaded and, e.g., replace "." with "_"?

The data has been transferred to ml_training folder so someone with write/read access rights needs to this, and also the sanitation needs to take place also for all the v3 zarr files. It is not a challenge or anything per se to have a script to do this automatically, or something that an agent cannot do fast and clean, but I find it time consuming, even only the effort on pinging people just for a ".".
Also to be fair I do not really find self explained having 2.5 as 2_5. What do you think?

No, I mean when we read in the training log files we should santize the column name to a valid format.

I tried, if you sanitize the column names then the legends are affected necessarily:

Also you cannot avoid but to have something of the sort to correct the titles. I believe sanitizing is much more prune to bugs/errors in the future.

                            elif len(col_split) == 4:
                                if (
                                    col_split[1].lower() == stream_name.lower()
                                    and col_split[2].lower() == err.lower()
                                    and col_split[3] == channel_sanitized
                                ):
                                    data_cols += [col]
                                    title_col = (
                                        f"LossPhysical.{stream_name}.{err}.{channel_display}"                 <---------
                                        if title_col is None
                                        else title_col
                                    )
                            elif len(col_split) == 5:
                                if (
                                    col_split[1].lower() == stream_name.lower()
                                    and col_split[2].lower() == err.lower()
                                    and col_split[3] == channel_sanitized
                                    and int(col_split[4]) in forecast_steps
                                ):
                                    data_cols += [col]
                                    title_col = (
                                        f"LossPhysical.{stream_name}.{err}.{channel_display}"               <---------
                                        if title_col is None
                                        else title_col
                                    )

But is it an issue to have ...thetao_2_5 instead of ...thetao_2.5 ? plot_train is an absolute expert system and the 3 people that would be affected would know, not?

Hello, I understand, I committed the second solution, that sanitizes the logging data. I used the term "sanitize" in the functions but it might be that another term would be better. Let me know if this meets your request better :)

[SavvasMel]

Are we still interested for this to be merged or should I close the issue?

[TillHae]

@SavvasMel @clessig I stumbled across this, because the issue was still open. Maybe the best idea would be to just fix how we select data_cols and handle title_col completely.

If you look at how it is currently implemented, we try to reverse-engineer column names by splitting on . and checking len(col_split) < 4, == 4, or == 5. That inevitably fails whenever a channel name contains a dot (like avg_thetao_2.5).

Instead of reverse-engineering with string splits, we already know the exact stream_name, err, and channel we are looking for from the loops. We can just construct the expected column string directly (e.g. ...{stream_name}.{err}.{channel}) and check whether the column ends with that suffix directly or with an explicit forecast step appended.

In general, if you take a deeper look at how weird the entire handling of title_col is, we don't even need it. It only exists to chop off the forecast step on line 525 with .split(".")[:-1] to set the chart title. This is something we could already set directly from our loop variables without any of that.

We should probably think about cleaning up this entire 5-nested-for-loop "architecture", but doing a direct string match instead of splitting on dots should immediately fix the bug without touching train_logger.py or renaming columns in the DataFrame.

What do you think?

[clessig]

@SavvasMel @clessig I stumbled across this, because the issue was still open. Maybe the best idea would be to just fix how we select data_cols and handle title_col completely.

If you look at how it is currently implemented, we try to reverse-engineer column names by splitting on . and checking len(col_split) < 4, == 4, or == 5. That inevitably fails whenever a channel name contains a dot (like avg_thetao_2.5).

Instead of reverse-engineering with string splits, we already know the exact stream_name, err, and channel we are looking for from the loops. We can just construct the expected column string directly (e.g. ...{stream_name}.{err}.{channel}) and check whether the column ends with that suffix directly or with an explicit forecast step appended.

In general, if you take a deeper look at how weird the entire handling of title_col is, we don't even need it. It only exists to chop off the forecast step on line 525 with .split(".")[:-1] to set the chart title. This is something we could already set directly from our loop variables without any of that.

We should probably think about cleaning up this entire 5-nested-for-loop "architecture", but doing a direct string match instead of splitting on dots should immediately fix the bug without touching train_logger.py or renaming columns in the DataFrame.

What do you think?

@TillHae I agree that the problem is encoding too many things in strings, without having properly thought about a convention that avoids collisions. But we shouldn't try to solve this now.

It sounds like you have a clear idea on how to set up a PR that constructs the col names from the available info and that avoids the problem. Could you implement it?

[TillHae]

@SavvasMel @clessig I stumbled across this, because the issue was still open. Maybe the best idea would be to just fix how we select data_cols and handle title_col completely.

If you look at how it is currently implemented, we try to reverse-engineer column names by splitting on . and checking len(col_split) < 4, == 4, or == 5. That inevitably fails whenever a channel name contains a dot (like avg_thetao_2.5).

Instead of reverse-engineering with string splits, we already know the exact stream_name, err, and channel we are looking for from the loops. We can just construct the expected column string directly (e.g. ...{stream_name}.{err}.{channel}) and check whether the column ends with that suffix directly or with an explicit forecast step appended.

In general, if you take a deeper look at how weird the entire handling of title_col is, we don't even need it. It only exists to chop off the forecast step on line 525 with .split(".")[:-1] to set the chart title. This is something we could already set directly from our loop variables without any of that.

We should probably think about cleaning up this entire 5-nested-for-loop "architecture", but doing a direct string match instead of splitting on dots should immediately fix the bug without touching train_logger.py or renaming columns in the DataFrame.

What do you think?

@TillHae I agree that the problem is encoding too many things in strings, without having properly thought about a convention that avoids collisions. But we shouldn't try to solve this now.

It sounds like you have a clear idea on how to set up a PR that constructs the col names from the available info and that avoids the problem. Could you implement it?

Yes, I will implement something in the upcoming days.