Bugfix for bug preventing the plotting of trajectories ending on exactly 0.00 longitude #18
Conversation
…y and ancillary trajectory or between two ancillary trajectories.
|
In the review we should check that all the checks are passing, precommit and all the tests. |
src/pytrajplot/parse_data.py
Outdated
| # check if plot_info has been found | ||
| if not "plot_info_dict" in dir(): | ||
| print("ERROR: plot_info file not found:", prefix_dict["plot_info"]) | ||
| raise |
There was a problem hiding this comment.
if no expression is present in the raise, it will re-raise the last exception raised:
https://docs.python.org/2/reference/simple_stmts.html?highlight=raise#raise
This is useful for catch blocks, if you want to do something but letting also the exception by caught by others later in the stack.
But in this context there is no exception being triggered, isnt? So it will most likely generate a TypeError, which seems to me not the intention.
I would replace by
raise RuntimeError(".....")
or equivalent exception and remove the print.
There was a problem hiding this comment.
I would also recommend adding a test that triggers and captures the exception, to make sure the logic and behaviour is correct
| @@ -82,6 +82,7 @@ def _get_traj_dict(data, number_of_trajectories=None, traj_length=None) -> dict: | |||
|
|
|||
| def _check_dateline_crossing( | |||
| lon: pd.Series, | |||
| number_of_times: int, | |||
There was a problem hiding this comment.
notice also the doc of the return type is wrong.
| if ( | ||
| not np.isnan(lon[flip_index]) | ||
| and not np.isnan(lon[flip_index - 1]) | ||
| and flip_index % number_of_times != 0 |
There was a problem hiding this comment.
I guess this is the core of the bug fix, and I am lacking knowhow to properly review.
I find number_of_times a confusing name that does not help understand the logic. From a different part of the code I read #lines that make up one trajectory. So would it be more appropriate to say number_of_segments ?
Is a trajectory composed by a number of segments with all same number of points and the condition does want to ignore when moving between segments?
If so, I would probably solve the problem differently by using a different data structure, a sequence of sequences for lon. So that the algorithms applied to only the inner most sequence can not mixed accross contiguous segments. That eliminates the need of this extra condition, and makes the intention more readable.
But I am not sure I understood it
There was a problem hiding this comment.
number_of_times is what is really meant, as a trajectory is composed of a (fixed per file) number of time steps, however restructuring the data as proposed could be beneficial --> created an issue for later
Updated versions of actions to avoid "Node.js 12 actions are deprecated" warnings.
Removed unnecessarily detailed versions.
Bugfix for bug preventing the plotting of trajectories when one ends with 0.00 longitude and next starts with abs(lon) > 20 as this was wrongly interpreted as crossing the date line.
Additionally, the long lines in the README.md are wrapped and some other minor edits are made. The patch level of the version is incremented by one. The license is changed to BSD 3-Clause License (the default for public MeteoSwiss projects). A minor bugfix has been made in the makefile (formerly Makefile). As this might be removed in a future version, use the --dry-run option to make to see what you would have to do manually without it.