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

Add another design cost #190

Closed
NicolaCourtier opened this issue Feb 9, 2024 · 6 comments · Fixed by #191
Closed

Add another design cost #190

NicolaCourtier opened this issue Feb 9, 2024 · 6 comments · Fixed by #191
Assignees
Labels
enhancement New feature or request

Comments

@NicolaCourtier
Copy link
Member

Feature description

Add another design optimisation example, for example maximising the volumetric energy density.

Motivation

The motivation is to provide an alternative to the existing GravimetricEnergyDensity for comparison purposes and applications where size is more important than weight.

Possible implementation

Add VolumetricEnergyDensity and the required cell_volume function to the base electrochemical model.

Additional context

No response

@NicolaCourtier NicolaCourtier added the enhancement New feature or request label Feb 9, 2024
@NicolaCourtier NicolaCourtier self-assigned this Feb 9, 2024
@NicolaCourtier NicolaCourtier linked a pull request Feb 9, 2024 that will close this issue
@BradyPlanden
Copy link
Member

BradyPlanden commented Feb 9, 2024

Nice, that will be very helpful!

I took a quick look at the open PR for this and was thinking about the best way to limit the number of new cost classes we need to add as we move forward. One way would be to merge GravimetricEnergyDensity and VolumetricEnergyDensity into something like EnergyDensityCost and provide an optional argument to select the appropriate geometric reference (i.e. ref=volume or ref=mass). This would reduce the number of lines we have to maintain and give us a clean interface to extend. Any thoughts?

@NicolaCourtier
Copy link
Member Author

I would like to add more design costs over time. I guess that these two costs could be merged for example, but I would argue that this would be at the expense of user-friendliness? I think it's helpful to be able to see what the cost is from its name. My approach would be to split the cost classes like we do the models. What do you think?

@BradyPlanden
Copy link
Member

I think it makes sense to merge those that result in a high amount of common structure, which I think these do, right? Code reuse will help us a lot as new developers get involved (so many fewer bugs + easier to review PRs). For future design costs that differ from the above classes, I would agree to split into different classes as we do in models.

The alternative would be to create a common base class, but this adds complexity for developers to understand, so I would probably prefer to limit the number of subclasses we have. Might be a good compromise though?

@NicolaCourtier
Copy link
Member Author

Common base classes, like we have for models, would work for me. There will be other costs that require rebuilding of the model like these energy density costs do. So I think it makes sense to group design costs this way, rather than just pairing up the energy densities. Plus this way, we can keep the explicit names for each cost.

@NicolaCourtier
Copy link
Member Author

Shall I create a costs folder containing fitting and design cost base classes as part of this PR?

@BradyPlanden
Copy link
Member

Agreed, it's probably best to move the costs back into a directory with this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants