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

Abstract simulation class prototypes as excuse to get coffee #753

Merged
merged 10 commits into from Apr 14, 2020

Conversation

namurphy
Copy link
Member

@namurphy namurphy commented Feb 3, 2020

The stated purpose of this pull request is to create prototype abstract interfaces for broad classes of plasma simulations. The actual purpose is to give me an excuse to get a coffee and vegan banana muffin.

The idea is to enable us to "program to an interface, not an implementation," whilst being sufficiently caffeinated. If a variety of plasma simulation packages program to a standard simulation interface, then it would make it much more straightforward to switch between different simulation and analysis techniques.

What is here just now is just a start. We'll need to think about this more carefully, especially what information should be included/required and what we should name each of these abstract methods and attributes. The names should avoid ambiguity. Right now I'm wondering if end_time could be interpreted as the actual end time of the simulation, or as the end time that the user inputs to tell the simulation when to stop. We probably also want to keep the parts related specifically to the numerical method separate from the parts related specifically to the physical problem setup (which should be independent of numerical method).

This pull request is brought to you by cup of coffee number 55 ± 5 (1σ) that I've had in my life.

Cross-references: #603, #662, #665, #714, PlasmaPy/PlasmaPy-PLEPs#24

@pep8speaks
Copy link

pep8speaks commented Feb 3, 2020

Hello @namurphy! Thanks for updating your pull request.

Congratulations! There are no PEP8 issues in this pull request. 😸

Comment last updated at 2020-04-02 18:38:32 UTC

@StanczakDominik
Copy link
Member

Hey, is everything all right?

@codecov
Copy link

codecov bot commented Feb 3, 2020

Codecov Report

Merging #753 into master will decrease coverage by 0.09%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #753     +/-   ##
=========================================
- Coverage   96.26%   96.17%   -0.1%     
=========================================
  Files          55       56      +1     
  Lines        4957     4988     +31     
=========================================
+ Hits         4772     4797     +25     
- Misses        185      191      +6
Impacted Files Coverage Δ
plasmapy/simulation/abstractions.py 76.47% <76.47%> (ø)
plasmapy/formulary/dispersionfunction.py 100% <0%> (ø) ⬆️
plasmapy/formulary/distribution.py 100% <0%> (ø) ⬆️
plasmapy/utils/exceptions.py 100% <0%> (ø) ⬆️
plasmapy/diagnostics/langmuir.py 93.21% <0%> (ø) ⬆️
plasmapy/utils/pytest_helpers/pytest_helpers.py 93.4% <0%> (ø) ⬆️
plasmapy/classes/exceptions.py 100% <0%> (ø) ⬆️
plasmapy/formulary/quantum.py 65.85% <0%> (ø) ⬆️
plasmapy/classes/sources/plasma3d.py 100% <0%> (ø) ⬆️
plasmapy/utils/roman/roman.py 100% <0%> (ø) ⬆️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8303ca0...83994c0. Read the comment docs.

@namurphy
Copy link
Member Author

namurphy commented Feb 3, 2020

Hey, is everything all right?

I accidentally based this off of a different branch so I had to rebase, and then made a few other mistakes. I'm getting an error I don't understand though:

============================================================= 2680 passed, 5 xfailed, 127 warnings, 2 errors in 56.24s =============================================================

ERROR: InvocationError for command /home/travis/build/PlasmaPy/PlasmaPy/.tox/py37/bin/pytest -vvv --pyargs plasmapy --cov=plasmapy --cov-config=/home/travis/build/PlasmaPy/PlasmaPy/setup.cfg --durations=25 /home/travis/build/PlasmaPy/PlasmaPy/docs -n=auto --dist=loadfile --ignore=/home/travis/build/PlasmaPy/PlasmaPy/docs/conf.py (exited with code 1)

___________________________________ summary ____________________________________
ERROR:   py37: commands failed
The command "tox -e $TOX_ENV $TOX_ARG" exited with 1.
1.06s$ pip install --upgrade codecov
Collecting codecov
  Downloading https://files.pythonhosted.org/packages/8b/28/4c1950a61c3c5786f0f34d643d0d28ec832433c9a7c0bd157690d4eb1d5f/codecov-2.0.15-py2.py3-none-any.whl
Requirement already satisfied, skipping upgrade: requests>=2.7.9 in /home/travis/miniconda/lib/python3.7/site-packages (from codecov) (2.22.0)
Collecting coverage (from codecov)
  Using cached https://files.pythonhosted.org/packages/9a/08/7fa92679a903cac0e01a7fee2cadd92089d96a3002c34e8f2295b80d6b68/coverage-5.0.3-cp37-cp37m-manylinux1_x86_64.whl
Requirement already satisfied, skipping upgrade: urllib3!=1.25.0,!=1.25.1,<1.26,>=1.21.1 in /home/travis/miniconda/lib/python3.7/site-packages (from requests>=2.7.9->codecov) (1.24.2)
Requirement already satisfied, skipping upgrade: certifi>=2017.4.17 in /home/travis/miniconda/lib/python3.7/site-packages (from requests>=2.7.9->codecov) (2019.6.16)
Requirement already satisfied, skipping upgrade: idna<2.9,>=2.5 in /home/travis/miniconda/lib/python3.7/site-packages (from requests>=2.7.9->codecov) (2.8)
Requirement already satisfied, skipping upgrade: chardet<3.1.0,>=3.0.2 in /home/travis/miniconda/lib/python3.7/site-packages (from requests>=2.7.9->codecov) (3.0.4)
Installing collected packages: coverage, codecov
Successfully installed codecov-2.0.15 coverage-5.0.3
The command "pip install --upgrade codecov" exited with 0.

😕

@StanczakDominik
Copy link
Member

I meant the coffee stuff, it sounded very much like a "no I have absolutely not been kidnapped, thank you" kind of situation!

Run tox locally and see if it works? I don't see any obvious reasons why it would stop yet.

@namurphy
Copy link
Member Author

namurphy commented Feb 3, 2020

I meant the coffee stuff, it sounded very much like a "no I have absolutely not been kidnapped, thank you" kind of situation!

Oh! Yep, I'm alright. A friend introduced me to coffee last year which I had never really drunk before, and I'm just now able to order plain coffee at a coffee shop while sounding like I know what I'm doing (but not any of those fancy coffee drinks which I still have trouble with).

Run tox locally and see if it works? I don't see any obvious reasons why it would stop yet.

I'll try this...thanks!

@namurphy
Copy link
Member Author

namurphy commented Feb 3, 2020

Okay, good! Things seem to be working better now. The problem was related to me having created the branch based on an obsolete version of plasmapy/master on my laptop. I absentmindedly forgot to do a git fetch --all beforehand. Let this be a lesson to us all on the perils of too much sugar and caffeine!

@namurphy
Copy link
Member Author

namurphy commented Feb 4, 2020

Before I forget...one of my (non-coffee related) goals is for us to have a set of prototype abstract interfaces for different aspects of numerical simulations put together by our 0.4.0 release. Once those are ready, we will be able to ask for feedback from the broader plasma simulation community in order to iterate on the interfaces in subsequent releases.

We can also open up the conversation about creating open metadata standards (e.g., openPMD) for the different aspects of simulations. To quote myself from the Houston Community Planning Process meeting last month...

Grumble grumble we need open metadata standards!!!!! grumble grumble.

@namurphy namurphy added this to the v0.4.0 milestone Feb 5, 2020
Copy link
Member

@StanczakDominik StanczakDominik left a comment

Choose a reason for hiding this comment

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

def time_advance(self) -> NoReturn:
"""Perform one step of the time advance."""

pass
Copy link
Member

Choose a reason for hiding this comment

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

Putting these in as abstractmethods means they're required, and any simulation we end up implementing must have this sort of logic:

while time < self.end_time():
	self.time_step()
	self.time_advance()

For a simulation of, say, 1e10 steps, that's a lot of Python function calls and a lot of time we'd be spending just calling these two functions, whatever they are. Then we cannot wrap this in Numba:

@numba.njit
def run(...):
    while time < end_time: 
        time_step(...)
        time_advance(...)

which, for some simulations, could be a massive time saver.

I'm aware that I changed the interface pretty seriously; this may be something we can avoid with JITclass, but I still haven't started figuring that out except for "it's not as good as it could be right now". Though this part seems new and promising:

All methods of a jitclass are compiled into nopython functions. The data of a jitclass instance is allocated on the heap as a C-compatible structure so that any compiled functions can have direct access to the underlying data, bypassing the interpreter.

So maybe it's viable, doing it that way! I don't know just yet. Further research necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, those are good points. There is also the use case that someone may wish to have the simulate or run method call a Fortran or C code and essentially do nothing in Python.

I'm wondering if we would want to have a hierarchy of abstract base classes of varying specificity. The most generic one could have the initialize, simulate, and finalize abstract methods and nothing else. A more specific one could have time_step, time_advance, and so on. Having them be functions in order to facilitate JIT compiling is an interesting thought, which is reminding me of Julia's approach. I'll have to look into numba.jitclass too.

In any case, everything I put down so far is in the brainstorm phase, as we'll need to have a few discussions to figure out what should go in there. I'm also wondering if some of the things we want to include should be in a metadata schema rather than an abstract base class. Hm...

Copy link
Member

Choose a reason for hiding this comment

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

I did look into jitclass a bit and I think it's going to work with this kind of approach. I'll upload the notebook later and I can show you at the telecon

What we could do, though, is to limit the ABC part to a more user facing part of the interface. It's not necessary to allow single time steps to be run externally, though it'd be nice for debugging, I guess; but initial conditions, running the loop, data save and load and possibly restart from saved data (probably some more, too) would be pretty consistently necessary for simulations.

Copy link
Member

Choose a reason for hiding this comment

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

I did look into jitclass a bit and I think it's going to work with this kind of approach. I'll upload the notebook later and I can show you at the telecon

What we could do, though, is to limit the ABC part to a more user facing part of the interface. It's not necessary to allow single time steps to be run externally, though it'd be nice for debugging, I guess; but initial conditions, running the loop, data save and load and possibly restart from saved data (probably some more, too) would be pretty consistently necessary for simulations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've also been wondering about how the different ABCs will fit together. I've been thinking for a while about how we need one each for initial conditions, boundary conditions, and the computational domain. We also need one to allow us to work with simulation results.

There are probably also going to be some components that users may not end up needing...like a data save and load may not be necessary for a short simulation that only gets interpreted interactively.

So many things to think about! 🙀

@namurphy
Copy link
Member Author

namurphy commented Apr 2, 2020

Following @StanczakDominik's comments above, I decided to tentatively remove abstract methods like start_time and end_time. It's shorter and simpler now, which is probably better for the time being. This is ready for another round of code review.

@namurphy namurphy merged commit 1942209 into PlasmaPy:master Apr 14, 2020
@namurphy namurphy deleted the abstract-simulation branch August 14, 2021 13:03
@namurphy namurphy added plasmapy.simulation Related to the plasmapy.simulation subpackage and removed plasmapy.simulations labels May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plasmapy.simulation Related to the plasmapy.simulation subpackage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants