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

Remove SolverABC #1625

Merged
merged 3 commits into from
Nov 11, 2022
Merged

Conversation

hsaunders1904
Copy link
Contributor

Linked Issues

Closes #1423

Description

Removes SolverABC and incorporates its functionality into CodesSolver. Fortunately, SolverABC wasn't used in many places.

I do think we may be missing a piece of the puzzle, in that we have Builders that make CAD, Designers that design CAD, but we don't have a base 'calculator'. It could be that we don't actually need one, but I think it's worth a discussion at some point. #1611 contains one suggestion.

Interface Changes

No more SolverABC.

Checklist

I confirm that I have completed the following checks:

  • Tests run locally and pass pytest tests --reactor
  • Code quality checks run locally and pass flake8 and black .
  • Documentation built locally and checked sphinx-build -W documentation/source documentation/build

With the new Builder/Designer architecture, this Solver base class no
longer has a place.

All the functionality of the base class has been moved to the
CodesSolver class, which is the only place the base class was really
being used.
Make it so it no longer derives from SolverABC, which has been removed
@hsaunders1904 hsaunders1904 added the base Tasks relating to the base module label Nov 9, 2022
@hsaunders1904 hsaunders1904 requested review from a team as code owners November 9, 2022 13:31
@sonarcloud
Copy link

sonarcloud bot commented Nov 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@hsaunders1904
Copy link
Contributor Author

hsaunders1904 commented Nov 9, 2022

I've just noticed that RunMode is used in equilibria, but I've moved it into codes. This probably needs to be resolved before we can think about merging this.

Edit: although, the transport solver is using a CodesSolver from codes anyway. What do you think, @je-cook?

@je-cook
Copy link
Contributor

je-cook commented Nov 9, 2022

I've just noticed that RunMode is used in equilibria, but I've moved it into codes. This probably needs to be resolved before we can think about merging this.

Edit: although, the transport solver is using a CodesSolver from codes anyway. What do you think, @je-cook?

We could put them both under an if typing.TYPE_CHECKING: as a quick thing. It may be quite difficult to resolve otherwise.

If we have better solutions than that I am open to them but I imagine we will come across these typing dependency things quite regularly.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

⚠️ Warning Report

Found 0 new warnings, 0 fixed warnings. 🎉

@hsaunders1904
Copy link
Contributor Author

We could put them both under an if typing.TYPE_CHECKING: as a quick thing. It may be quite difficult to resolve otherwise.

The concern is more of an architectural one - modules should be as 'standalone' as we can make them, and, where possible, only rely on things in base and probably utilities. In theory, we could definitely have a transport solver that's not a CodesSolver in the case of fem_fixed_boundary.

Since I noticed that the dependency on codes was already there, I'm less worried about this being merged. But I think we should consider this issue as part of #1611 (or open a new ticket)

Copy link
Contributor

@je-cook je-cook left a comment

Choose a reason for hiding this comment

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

I'm happy with this as is with the open issues.

It may be nice to have a CodeSolver example that isnt dependent on having any code installed. Maybe instead of getting that in here we should open an issue for that. I think it could be made from the example thats deleted here.

@je-cook je-cook merged commit 2ec7ffd into develop Nov 11, 2022
@je-cook je-cook deleted the hsaunders1904/1423_move_SolverABC_out_of_base_module branch November 11, 2022 09:52
ivanmaione pushed a commit that referenced this pull request Nov 18, 2022
* Remove SolverABC

With the new Builder/Designer architecture, this Solver base class no
longer has a place.

All the functionality of the base class has been moved to the
CodesSolver class, which is the only place the base class was really
being used.

* Refactor EU-DEMO SteadyStatePowerCycle

Make it so it no longer derives from SolverABC, which has been removed

* Remove references to SolverABC in docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base Tasks relating to the base module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move SolverABC out of base module
2 participants