Skip to content

Refactoring unit implementation for use in dataframes and plots - #88

Merged
DunklesArchipel merged 33 commits into
echemdb:mainfrom
DunklesArchipel:entrydf
Jan 7, 2022
Merged

Refactoring unit implementation for use in dataframes and plots#88
DunklesArchipel merged 33 commits into
echemdb:mainfrom
DunklesArchipel:entrydf

Conversation

@DunklesArchipel

Copy link
Copy Markdown
Member

The data frames in the database do not contain any information on the currently used units. We create the data frame from the CSV file, which has been created with SI units.

In our previous version, the data frame could be created with units different from SI. However, the returned df obviously does not contain this information.

Such information is crucial for the plot module, where the units should be printed on the x and y-axis labels.

Another issue is that the y axis can be either a current (I) or a current density (j) with distinctly different units.

x and y units are now returned by xy_units(). It is possible to return SI units, custom units or the original units from the figure.

Maybe there is a smarter quick and dirty solution than this one. Otherwise, we could wait for pandas 2.0 where unit storage along with data frames is eventually implemented.

Also the rendering of the Figure on the cv_entry.md has been updated. (Not yet super nice, but better than before.)

@saraedum

saraedum commented Dec 4, 2021

Copy link
Copy Markdown
Member

Do you have a link for that pandas update? I also would not mind to use an experimental change in pandas here.

@saraedum

saraedum commented Dec 5, 2021

Copy link
Copy Markdown
Member

Do you have a link for that pandas update? I also would not mind to use an experimental change in pandas here.

I could not find any actual pandas 2 implementation. It seems to be just a rough roadmap.

@saraedum

saraedum commented Dec 5, 2021

Copy link
Copy Markdown
Member

Have you had a look at astropy time series? They support quantities, see https://docs.astropy.org/en/stable/timeseries/pandas.html.

Comment thread echemdb/data/cv/entry.py Outdated

@saraedum saraedum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I proposed a way to get rid of the I/j hack. Otherwise, this looks great apart from some wording issues, e.g., I am pretty sure that the lines in docstrings that end in :: should not end in a ..

Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
return Descriptor(self.package.descriptor)[name]

def df(self, yunit=None):
def xy_units(self, xunit=None, yunit=None):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't split this into x_unit and y_unit? There seems to be no interaction between the two parts of this method.

Comment thread echemdb/data/cv/entry.py Outdated
Comment on lines +136 to +139
if 'j' in column_names:
yunit = u.A / u.m**2
if 'I' in column_names:
yunit = u.A

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This if-j-if-I is really annoying.

Could we do the following: We have x_unit and y_unit which return the units of the axes, as you planned here. Then we have x and y which return the name of the axes, i.e., U and I/j.

Suggested change
if 'j' in column_names:
yunit = u.A / u.m**2
if 'I' in column_names:
yunit = u.A
if self.y == 'j':
yunit = u.A / u.m**2
elif self.y == 'I':
yunit = u.A
else:
raise NotImplementedError("Unexpected naming of y axis.")

Also, we should always catch unexpected values as early as possible, as this makes debugging much easier when something goes wrong.

Comment thread echemdb/data/cv/entry.py Outdated
Comment on lines +255 to +265
if 'j' in df.columns:
fig.add_trace(plotly.graph_objects.Scatter(x=df['U'], y=df['j'], mode='lines'))
ylabel = f'j [{str(yunit)}]'
if 'I' in df.columns:
fig.add_trace(plotly.graph_objects.Scatter(x=df['U'], y=df['I'], mode='lines'))
ylabel = f'I [{str(yunit)}]'

fig.update_layout(template="simple_white", showlegend=False, autosize=True, width=600, height=400,
margin=dict(l=70, r=70, b=70, t=70, pad=7),
xaxis_title=f"U [{xunit}]",
yaxis_title=ylabel)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if 'j' in df.columns:
fig.add_trace(plotly.graph_objects.Scatter(x=df['U'], y=df['j'], mode='lines'))
ylabel = f'j [{str(yunit)}]'
if 'I' in df.columns:
fig.add_trace(plotly.graph_objects.Scatter(x=df['U'], y=df['I'], mode='lines'))
ylabel = f'I [{str(yunit)}]'
fig.update_layout(template="simple_white", showlegend=False, autosize=True, width=600, height=400,
margin=dict(l=70, r=70, b=70, t=70, pad=7),
xaxis_title=f"U [{xunit}]",
yaxis_title=ylabel)
fig.add_trace(plotly.graph_objects.Scatter(x=df[self.x], y=df[self.y], mode='lines'))
fig.update_layout(template="simple_white", showlegend=False, autosize=True, width=600, height=400,
margin=dict(l=70, r=70, b=70, t=70, pad=7),
xaxis_title=f"{self.x} [{xunit}]",
yaxis_title=f"{self.y} [{yunit}]")

saraedum and others added 16 commits December 6, 2021 02:31
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Still need to create correct doctests
This function will change once echemdb/svgdigitizer#106 is implemented.
@DunklesArchipel

Copy link
Copy Markdown
Member Author

@saraedum I hopefully implemented all your changes.

The doctests fail probably due to the latest update on svgdigitizer that you did. I cannot reproduce that particular doctest error locally, even with the version of the svgdigitizer.

@DunklesArchipel DunklesArchipel linked an issue Dec 20, 2021 that may be closed by this pull request
@saraedum

saraedum commented Dec 21, 2021

Copy link
Copy Markdown
Member

@saraedum I hopefully implemented all your changes.

The doctests fail probably due to the latest update on svgdigitizer that you did. I cannot reproduce that particular doctest error locally, even with the version of the svgdigitizer.

Yes, I think that's the problem described at https://github.com/echemdb/website/blob/main/environment.yml#L30.

To reproduce, I think you have to delete the generated/ directory somewhere that holds a local cache of digitized data.

Should be fixed by #96.

@saraedum

Copy link
Copy Markdown
Member

There are merge conflicts @DunklesArchipel .

@linuxrider linuxrider left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

@saraedum saraedum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great. I have just some minor comments. Feel free to merge once addressed.

Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py
Comment thread echemdb/data/cv/entry.py
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
Comment thread echemdb/data/cv/entry.py Outdated
DunklesArchipel and others added 8 commits January 7, 2022 11:12
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
Co-authored-by: Julian Rüth <julian.rueth@fsfe.org>
@DunklesArchipel
DunklesArchipel merged commit c7d68e2 into echemdb:main Jan 7, 2022
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.

Create plots with original axis units

3 participants