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

Basic plasma factory implementation #459

Merged
merged 8 commits into from May 27, 2018

Conversation

7 participants
@ritiek
Contributor

ritiek commented May 16, 2018

We can now instantiate the expected plasma subclass based on input keyword arguments.

>>> import astropy.units as u
>>> import numpy as np
>>> import plasmapy.classes

>>> T_e = 25 * 15e3 * u.K
>>> n_e = 1e26 * u.cm ** -3
>>> Z = 2.0
>>> particle = 'p'
>>> blob = plasmapy.classes.Plasma(T_e=T_e,
...                                n_e=n_e,
...                                Z=Z,
...                                particle=particle)

>>> type(blob)
plasmapy.classes.sources.plasmablob.PlasmaBlob

>>> three_dims = plasmapy.classes.Plasma(domain_x=np.linspace(0, 1, 3) * u.m,
...                                      domain_y=np.linspace(0, 1, 3) * u.m,
...                                      domain_z=np.linspace(0, 1, 3) * u.m)

>>> type(three_dims)
plasmapy.classes.sources.plasma3d.Plasma3D

I am not sure if relying solely on keyword arguments is good enough to assume the plasma subclass but I don't have a better idea either (SunPy uses different parameters for data and header, I don't know how well will it work in our case).

We also need to add common plasma data to GenericPlasma and as of now PlasmaFactory does nothing but just inherits from BasicRegistrationFactory.

@pep8speaks

This comment has been minimized.

pep8speaks commented May 16, 2018

Hello @ritiek! Thanks for updating your pull request.

Line 8:1: E302 expected 2 blank lines, found 1

Line 13:1: E302 expected 2 blank lines, found 1

Comment last updated on May 27, 2018 at 10:50 Hours UTC

@ritiek ritiek changed the title from Basic plasma factory implementation to [WIP] Basic plasma factory implementation May 16, 2018

@namurphy namurphy added this to the v0.2.0 milestone May 16, 2018

@namurphy namurphy added this to To do in PlasmaPy 0.2.0 via automation May 16, 2018

@ritiek

This comment has been minimized.

Contributor

ritiek commented May 17, 2018

So, how should we go about implementing GenericPlasma? What common plasma data would it deal with?

@lemmatum

This comment has been minimized.

Contributor

lemmatum commented May 17, 2018

If GenericPlasma is supposed to be a particular instance of a plasma I would rename it to ClassicalPlasma and make it deal with the high temperature, low density regime (weak coupling, sufficient particles in the Debye sphere, etc.). If instead GenericPlasma is going to be used to create various subclasses and it is just a container for shared properties between different kinds of plasmas then I would include things like electronTemperature, ionTemperature, electronDensity, ionDensity, averageIonization, etc.

Thoughts @namurphy @StanczakDominik ?

@namurphy

This comment has been minimized.

Member

namurphy commented May 18, 2018

The output from an magnetohydrodynamic simulation will generally include the magnetic field vector B, the velocity vector V, the current density vector J, the number density of particles per unit volume n (or sometimes the mass density ρ), the plasma pressure p, and the temperature T. Probably each individual data file will have information on a single time, and there will usually be multiple files. The simulations will sometimes be 2D and sometimes be 3D, and the grid positions will need to be recorded too. Sometimes the data will be in rectangular coordinates and other times it'll be in cylindrical coordinates, or even another coordinate system. Occasionally there will be other quantities that need to be included, like the magnetic vector potential A (which will reduce to a scalar in 2D). Simulation output will sometimes have units (and thus will need to be astropy.units.Quantity instances) and other times will be dimensionless (and thus will end up being numpy.ndarray instances).

Perhaps a good thing to do for developing the general framework would be to create your own data to try to read in and set up the appropriate subclass. It would be possible to create some mock HDF5 files using h5py that contain plasma data on a 2D or 3D grid, where the names of the variables in the HDF5 files correspond to the above variables. The Plasma class could then select the appropriate subclass from maybe Plasma2D and Plasma3D based on the information contained within the files.

Another possibility would be to create a class to represent data sets that use the OpenPMD standard (see also #167 and PlasmaPy/PlasmaPy-PLEPs#13), and make it so that Plasma is able to select that appropriate subclass.

@ritiek

This comment has been minimized.

Contributor

ritiek commented May 18, 2018

If instead GenericPlasma is going to be used to create various subclasses and it is just a container for shared properties between different kinds of plasmas then I would include things like electronTemperature, ionTemperature, electronDensity, ionDensity, averageIonization, etc.

This, most likely.

It would be possible to create some mock HDF5 files using h5py that contain plasma data on a 2D or 3D grid, where the names of the variables in the HDF5 files correspond to the above variables.

Nice, I guess I'll mess around with some HDF5 files for a bit.

from abc import ABC
# GenericPlasma subclass registry
PLASMA_CLASSES = OrderedDict()

This comment has been minimized.

@ritiek

ritiek May 18, 2018

Contributor

Do we really need to use an OrderedDict here or a simple dict object would suffice? SunPy seems to use OrderedDict but I am not sure about its usefulness (plus it is slower).

This comment has been minimized.

@Cadair

Cadair May 18, 2018

Contributor

Given you are Python 3.6+ you do not need OrderedDict here. Also I don't think it really matters at all.

This comment has been minimized.

@ritiek

ritiek May 18, 2018

Contributor

Ok, I guess then we should probably stick with dict atleast for now.

# GenericPlasma subclass registry
PLASMA_CLASSES = OrderedDict()
class GenericPlasmaRegistrar(ABC):

This comment has been minimized.

@Cadair

Cadair May 18, 2018

Contributor

I am assuming that you are planning on using this class for API definition and not just to be the keeper of the registry, so I would suggest you rename it to BasePlasma or something similar. As this is the class you will want to do isinstance with to test if it implements your plasma API

This comment has been minimized.

@ritiek

ritiek May 18, 2018

Contributor

I don't know for sure if we are going to use this class for API definition or not.

Based on my understanding, GenericPlasma is supposed to deal with data that is common in all plasmas. If we are going to have GenericPlasmaRegistrar do this task as well then we could remove GenericPlasma class and have this class renamed to BasePlasma and deal with both common plasma data and do the registration thing.

Thoughts @SolarDrew, @namurphy?

This comment has been minimized.

@Cadair

Cadair May 18, 2018

Contributor

It's a good idea to have an Abstract base class that dosen't implement any (much) API and then a base class which implements the abstract class in a way which is generic for plasmapy. This means that if anyone wants to do something very different down the line they can have all the benifits of the ABC and the registration without having to worry about any of the implementation in the generic class.

For an example of this see ndcube

This comment has been minimized.

@ritiek

ritiek May 18, 2018

Contributor

I think I understand now, that ndcube example is very interesting. So, we do both the registration thing and declare some abstract methods on the BasePlasma but define them in GenericPlasma. I'm good with that.

from abc import ABC
# GenericPlasma subclass registry
PLASMA_CLASSES = OrderedDict()

This comment has been minimized.

@Cadair

Cadair May 18, 2018

Contributor

I know this is how it's done in SunPy, but actually I would not have this as a module level constant. I would just have it as a class attribute on GenericPlasmaRegistrar

This comment has been minimized.

@ritiek

ritiek May 18, 2018

Contributor

Yep, good idea.

Plasma = PlasmaFactory(default_widget_type=GenericPlasma,
additional_validation_functions=['is_datasource_for'])
Plasma.registry = PLASMA_CLASSES

This comment has been minimized.

@Cadair

Cadair May 18, 2018

Contributor

Given my comment above, this would become GenericPlasma._registry (as it would be inherited from the registrar)

class GenericPlasma(GenericPlasmaRegistrar):
def __init__(self, **kwargs):
for key, value in kwargs.items():
setattr(self, key, value)

This comment has been minimized.

@Cadair

Cadair May 18, 2018

Contributor

I don't know if you have just implemented this as a stop-gap testing thing or not, but I would recommend not doing this in the long term.

import inspect
class BasicRegistrationFactory(object):

This comment has been minimized.

@Cadair

Cadair May 18, 2018

Contributor

oh the copy paste horror of it all 😝

you don't need (object) here

Plasma = PlasmaFactory(default_widget_type=GenericPlasma,
additional_validation_functions=['is_datasource_for'])
Plasma.registry = PLASMA_CLASSES

This comment has been minimized.

@Cadair

Cadair May 18, 2018

Contributor

Actually pass registry=GenericPlasma._registry to the PlasmaFactory constructor.

This comment has been minimized.

@ritiek

ritiek May 18, 2018

Contributor

That's nicer!

@SolarDrew

This comment has been minimized.

Contributor

SolarDrew commented May 20, 2018

Simulation output will sometimes have units (and thus will need to be astropy.units.Quantity instances) and other times will be dimensionless (and thus will end up being numpy.ndarray instances).

I disagree with the last point - I think if values are dimensionless they should use the dimensionless unit, both for consistency and to avoid ambiguity.

@ritiek

This comment has been minimized.

Contributor

ritiek commented May 20, 2018

Ah, yes. Astropy does have a dimensionless unit.

>>> u.m/u.m == u.dimensionless_unscaled
True
@SolarDrew

This comment has been minimized.

Contributor

SolarDrew commented May 20, 2018

Looks good to me so far 👍 Does anyone know what's up with the circleci failure? Is that the sphinx problem we've been having for a while?

@ritiek

This comment has been minimized.

Contributor

ritiek commented May 20, 2018

what's up with the circleci failure?

It probably has something to do with docs/plasma/index.rst. I'll see if I can fix it.

@ritiek ritiek force-pushed the ritiek:plasma-factory branch 2 times, most recently from bc30dd8 to 5006bce May 24, 2018

@ritiek ritiek force-pushed the ritiek:plasma-factory branch from 5006bce to 25345c5 May 24, 2018

@ritiek

This comment has been minimized.

Contributor

ritiek commented May 24, 2018

The coverage drop is because plasma_base.py has some uncovered methods. Everything else coverage related to this PR seems fine to me.

@SolarDrew

This comment has been minimized.

Contributor

SolarDrew commented May 24, 2018

Sounds reasonable to me. Any final opinions before I merge this then, @namurphy @StanczakDominik @lemmatum?

@StanczakDominik

This comment has been minimized.

Member

StanczakDominik commented May 24, 2018

Actually... Could you please hold off for now so we can finish 0.1.1 up on master? That'd also give me an opportunity to run a review here. I should be able to get this reviewed by today evening, barring unforeseen Linux errors.

@SolarDrew

This comment has been minimized.

Contributor

SolarDrew commented May 24, 2018

No problem @StanczakDominik , just let us know when you're ready. I think there is a workflow for keeping bugpatch changes separate from changes going into new releases though, but I can't remember what that is. I'll make an issue, since it's something that will probably come up again.

@StanczakDominik

This comment has been minimized.

Member

StanczakDominik commented May 24, 2018

Sure, that workflow is just not forgetting to merge bug fixes into stable, which I forgot to do :D

@SolarDrew

This comment has been minimized.

Contributor

SolarDrew commented May 24, 2018

Lol, sure, but I did think there was more logistical wizardry to it than that :P Let's move further conversation here: #474 :)

@StanczakDominik

This comment has been minimized.

Member

StanczakDominik commented May 24, 2018

Okay, reviewing this now because Linux decided to crap out on me again... let's just merge into master and I'll sort out the stable/unstable thing later.

@StanczakDominik StanczakDominik self-requested a review May 24, 2018

@StanczakDominik

I can't quite say I understood everything I just read, but I expect to grasp it once I've played with the code a bit. Looks good to go!

@ritiek

This comment has been minimized.

Contributor

ritiek commented May 27, 2018

By the way, do we need an __all__ list for plasma_base.py and plasma_factory.py?

@StanczakDominik

This comment has been minimized.

Member

StanczakDominik commented May 27, 2018

@StanczakDominik

This comment has been minimized.

Member

StanczakDominik commented May 27, 2018

All right, the master branch definitely needs more metaclass now. 😆

@StanczakDominik StanczakDominik merged commit 38087f6 into PlasmaPy:master May 27, 2018

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 96.017%
Details
ci/circleci: test-html Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

PlasmaPy 0.2.0 automation moved this from To do to Done May 27, 2018

@ritiek ritiek deleted the ritiek:plasma-factory branch Jun 2, 2018

@namurphy namurphy changed the title from [WIP] Basic plasma factory implementation to Basic plasma factory implementation Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment