Skip to content
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

Dataframe #16

Merged
merged 6 commits into from
Oct 20, 2023
Merged

Dataframe #16

merged 6 commits into from
Oct 20, 2023

Conversation

julian-evers
Copy link
Collaborator

No description provided.

@rtimms
Copy link
Collaborator

rtimms commented Oct 16, 2023

thanks, looks good. you could make it cleaner by doing something like

parameters = [
    "Volumetric stack energy density [Wh.L-1]",
    "Gravimetric stack energy density [Wh.kg-1]",
]
names = []
units = []
values = []
for parameter in parameters:
    name, unit = parameter.split("[")
    names.append(name.strip())
    units.append(unit.rstrip(']'))
    values.append(stack_ed[parameter])

data = {
    "Parameter": names,
    "Unit": units,
    "Value": values,
}

this would require you to have all the values in the units you want in the breakdown dicts, but i think it's cleaner as you don't need to manually specific a name and units separately - they are just taken from the key of the dictionary

pybamm_tea/tea.py Outdated Show resolved Hide resolved
name, unit = parameter.split("[")
names.append(name.strip())
units.append(unit.rstrip(']'))
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you need this? I don't think you need to have the subheadings as part of the dataframe

Copy link
Collaborator

@rtimms rtimms left a comment

Choose a reason for hiding this comment

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

thanks, looks good! Just a couple of minor changes and need to fix the merge conflicts then it's good to go

stack_ed.get("Gravimetric stack energy density [W.h.kg-1]"),
stack_ed.get("Stack average OCP [V]"),
stack_ed.get("Capacity [mA.h.cm-2]"),
10**6 * stack_ed.get("Stack thickness [m]"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just add "Stack thickness [um]" and "Stack density [kg.L-1]" to stack_ed too when you construct it, then you can just do the loop here too

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (33ea860) 80.99% compared to head (30e8b0b) 76.09%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #16      +/-   ##
==========================================
- Coverage   80.99%   76.09%   -4.91%     
==========================================
  Files           3        3              
  Lines         321      343      +22     
==========================================
+ Hits          260      261       +1     
- Misses         61       82      +21     
Files Coverage Δ
tests/test_tea.py 96.66% <100.00%> (ø)
pybamm_tea/tea.py 73.95% <32.35%> (-5.29%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

units = []
values = []
for parameter in parameters:
if "[" in parameter:
Copy link
Collaborator

Choose a reason for hiding this comment

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

alternatively, you could denote no units by "[-]"

@rtimms rtimms merged commit 8dcaecd into main Oct 20, 2023
9 of 11 checks passed
@julian-evers julian-evers deleted the dataframe branch December 10, 2023 10:51
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.

2 participants