ms_plotter works#176
Conversation
…y. This cannot be done closer to the dataseries since the measurement owns the dataseries and not referencing the dataseries?
… subsequent derivatives e.i grab fluxes for siq
…f changing data in ixdat
ScottSoren
left a comment
ScottSoren
left a comment
There was a problem hiding this comment.
Hi @AlexanderKrabbe ,
Great work. I like a lot that the manual unit conversions disappear from the plotting modules.
However, I'm surprised to see so much more code get added than removed. That indicates that pint units isn't giving us the desired simplification of ixdat yet.
Most of my comments are pointed towards that. I think a lot of the stuff added to units.py is not needed, but on the other hand some switches in units.py or data_series.py controlled by a variable defined in config.plugins could make it all work.
Another problem is that the development scripts that I have to test this on reactor data, demo_ixdat_reader_TPMS.py and demo_ms_spectrum_measurement.py don't work yet, presumably because the readers don't correctly set the units. Could you take a look at those and get them working? Let me know if you don't have the data.
Sorry this review comes so slow! No need to implement all my comments - just respond with your thoughts of how to proceed, unless a simplifying implementation is clear enough to do quickly. I don't think any substantial changes should be made which don't consist of a net decrease in code!
When I get your response to the comments, I'll merge it onto the units branch and we can take it from there.
| ) | ||
| self.replace_series(value_name, new_vseries) | ||
|
|
||
| def inplace_to_unit(self, valueseries_name, new_unit_name): |
There was a problem hiding this comment.
If this is a method of Measurement, its name should indicate that it applies to a single series. series_ito_unit or something.
Also, needs argument descriptions in the docstring.
When would you use it, though, given that one can always get a series out with getitem and then in-place convert the unit?
| self._selected_range = {"left": None, "right": None} | ||
|
|
||
| def new_ax(self, xlabel=None, ylabel=None, interactive=True): | ||
| def new_ax(self, xlabel=None, ylabel=None, interactive=True, use_quantity=False): |
There was a problem hiding this comment.
So use_quantity=True causes the method to ignore any xlabel and ylabel it is explicitly given? Are you sure that's the desired behavior?
Has to be explained in the docstring if so!
| """Initiate the ECMSPlotter with its default Meausurement to plot""" | ||
| super().__init__() | ||
| self.measurement = measurement | ||
| self.ureg = ureg |
There was a problem hiding this comment.
why does an msplotter object need its own reference to ureg? Can't one just use the global ureg everywhere? I would prefer that because it can cause confusion if it ever looks like units of two possibly different uregs are being compared.
|
|
||
| ureg.setup_matplotlib( | ||
| True | ||
| ) # I think this should be closer to the measurement or specific plotter |
There was a problem hiding this comment.
... or maybe in src/ixdat/config.py
What are the implications of each possible location?
| self.name = name or "" | ||
| try: | ||
| self.u = ureg(self.name) | ||
| self.u = ureg(self.name).u |
| ax.set_ylabel(f"signal / [{unit}]") | ||
| ax.set_xlabel(f"time / [{x_unit}]") | ||
|
|
||
| if logdata or (use_quantity and v.check(ureg.dimensionless)): |
There was a problem hiding this comment.
Please break this up into if + elif so the logic is easier to follow.
| ) | ||
|
|
||
| if use_quantity and x_unit and not x.check(x_unit): | ||
| error_message = ( |
There was a problem hiding this comment.
see previous comment on this.
Also is there a way to avoid repeating such similar code?
| return_quantity=use_quantity, | ||
| ) | ||
|
|
||
| if ureg("mol / s / cm ** 2").check(unit): |
There was a problem hiding this comment.
Is there a way to do somethign like
if "length^{-2}" in unit.dimensions: ...
?
Then there wouldn't have to be a repeat of this block for quantified vs non-quantified.
| ) | ||
| ylabel = f"{vu:~ixdat_log}" | ||
|
|
||
| x_mass = np.interp(t_v, t, x) |
There was a problem hiding this comment.
I know this isn't you but x_mass is a wierd name... looks like it should be called x_v (x for the specific value being plotted) to be consistent with the nomenclature t_v.
| stacklevel=2, | ||
| ax.xaxis.set_units(x_unit) | ||
|
|
||
| if unit and not logdata: |
There was a problem hiding this comment.
Again, there must be a way to avoid this...
2 participants
ms_plotter works
ms_plotter.vs() needs some clean up
The main thing is how pint lets you set a matplotlib formatter for the units, so when axes are passed on, the label dynamically updates if the unit of the axes shoul change:
ax = meas_ms.plot()
ax.yaxis.get_unit() -> "A"
ax.yaxis.set_units(ureg.nA)
plt.plot()
data is correctly plotted
There is a nice thing about pint with their class Context.
This class could be a nice way to provide context for the measurement when units changes from mol/s to mol/s/cm^2