implement a Field class similar to fields in Django, Marshmallow and DRF #68

Merged
merged 16 commits into from Oct 31, 2016

Projects

Done in Carousel-v0.3.5

2 participants

@mikofski
Contributor
  • should resolve #9 Fields layer (altho not really a layer)
  • add Field base class to core
  • add test_field to test_core to see if a class constructor works
  • add new test_fields to carousel.tests to try to make a data class using fields
@mikofski mikofski implement a Field class similar to fields in Django, Marshmallow and DRF
* should resolve #9 Fields layer (altho not really a layer)
* add Field base class to core
* add test_field to test_core to see if a class constructor works
* add new test_fields to carousel.tests to try to make a data class using fields
d3f8315
@mikofski mikofski added this to the v0.3 milestone Oct 14, 2016
@mikofski mikofski self-assigned this Oct 14, 2016
mikofski added some commits Oct 14, 2016
@mikofski mikofski refactor as Parameter class instead of Field
* consistent with parameters in each layer
* subclass to dict, easier than trying to reimplement dict, or change a lot of code, since all of the layers are already expecting parameters to be a dictionary
* do not create any attributes in Parameter, only dictionary items (if this changes later, make sure not to clash with members already defined in dict)
* add __repr__ method since dictionary is ugly
* add extras to collect items not in attributes (metadata)
* fix test_fields in test_core to use __getitem__ since it's a dict, instead of __getattribute__, aka getattr() as if it were an object class only
* fix comparison of velocity data elapsed_time was using wrong key for assertion comparison.
aba70dd
@mikofski mikofski fix data reader tests with new 'extra' key
* use DataParameter in tests
* use [f for f in get_fields(include_parents=False) if not f.auto_created)] to filter out auto created and related fields
* so don't need to import AutoField
* finally complete todo's in registry, just generate reg attrs from meta_names, can remove __init__(self) from most layers
* allow _private meta-names, why not?
* update docs
* in CommonBase set_param_file_or_parameters() only get Parameters
* add DataParameter class in data_sources and FormulaParameter to formulas
* remove test_field from test_core
9950336
@mikofski mikofski update registries, remove __init__, add docstrings for metadata
* add layer parameter classes, set _attrs
* if calc dependency is string, make it a list
* convert unc to percent
* remove field todo's in outputs
d95b4f2
@mikofski mikofski fix xlrd reader and test to use DataParameter
* address #43 all readers use same format
* update docs a little
* also have to fix apply units to cached data
* fix param_file must apply DataParameter to each item
* all test_data passsing! but need to add test for cached data!
80fc320
@mikofski mikofski add cached file to be used in testing 48654e0
@mikofski mikofski add meta attr to formulas and formula importer base class
* meta is Meta class, so module, package and path are attributes of it, not items
dad883b
@mikofski mikofski separate formulas, use Meta and FormulaParameter
* closes #64 - formulas are separated, no more formula keyword, all formulas are in parameters
* addresses #62 - formulas use Meta for module, package and path
* adresses #40 - very similar to #62, but now formulas has Meta too
* closed #6 - use meta_names to instantiate attributes for registries, this actually already happened in a recent commit
* extract Meta from parameter files if it's there using type("Meta", (), file_params.pop('Meta')) pretty easy
* read in formulas from param file as FormulaParameters instead of just dictionaries
* fix some of the pvpower example files: newstyle model and formula json files
4694f16
@mikofski mikofski move formulas_importer to class Meta
* remove formulas base constant _importer_attr, don't pop importer and don't set it
* in Formula, remove class formula importer class attribute, then check _meta for importer, and if None, set it to PyModuleImporter
* now formula importer is in _meta
* update tests in test_formula()
* in expression importer, 'expression' is in ['extras']
e3c0483
@mikofski mikofski update test_sim with Parameters 7a57ea8
@mikofski mikofski make simulation output folders recursively 850c6a8
@mikofski mikofski fix sim ID maker - no colon in filename f063e42
@mikofski mikofski add settings argument to simulations
* this is a HACK! need to get simulations to work with parameters, but to much work to implement #71
* sim parameter attributes are the simulation arguments
* settings arg used to select which set of sim args to use, if not given use first.
* update test_sim PythagorasSim() and Sandia_perfmod_newstyle
0911643
@mikofski mikofski use OutputParameter
* convert outputs param file to dict of OutputParameter
* convert simulation param file to SimParameter settings, use file name minus extension for settings name
* deprecated sim attributes will be in extras
* update tests and sandia_perfmod_newstyle with OutputParameter and SimParameter
47ee88f
@mikofski mikofski really heinous hacks to close #9
* fix uncertainty static calc conversion to percent is multiply by 100 (NOT divide st00pid)
* don't use CalcParameter yet, too much work, but leave it partially implemented, instead fake parameters in the constructor by getting the class attributes
* for test calc metaclass to pass, compare test1 to test2 instead of vv, since test 2 now has extra fields that are set by default, like "dynamic": [], that test1 doesn't have
* fix c_unc in comparison, array is unnecessary, get float64 using item(), since only one value
50b94d2
@anomam anomam was assigned by mikofski Oct 19, 2016
@mikofski
Contributor

@anomam this pr is ready for review, either put your comments here or look for the review button and click approve on the "Files Changed" tab or you can hunt for the review button - this help doc on reviews might help. thx and sorry it is so messy, it's a really heinous hack, not quite all of the v0.3 milestone stuff I wanted, but it had to be done.

carousel/tests/test_core.py
+ Test that Carousel field creates an object with attributes
+ """
+ Field._attrs = ['units', 'isconstant']
+ test_field = Field('my test field', units='W/m**2', isconstant=True)
@anomam
anomam Oct 19, 2016 Contributor

so here 'my test field' will be assigned to the 'units' attribute first? But then replaced by 'W/m**2' it looks like, is that correct?

@anomam
anomam approved these changes Oct 19, 2016 View changes
carousel/core/__init__.py
LOGGER.warning('This key: "%s" is not an attribute.', key)
+ super(Parameter, self).__init__(items, extras=extras)
@anomam
anomam Oct 19, 2016 Contributor

ok, so we're keeping the extra attribute fields entered. It's probably dealt with later but I was wondering what would happen to the attribute fields not entered when instantiating.

carousel/contrib/readers.py
@@ -19,9 +19,11 @@ def copy_model_instance(obj):
"""
https://djangosnippets.org/snippets/1040/
@anomam
anomam Oct 19, 2016 Contributor

snippet doesn't seem to work...
I'm trying to understand where '_meta' comes from right now. And 'get_fields' as well.

@mikofski
mikofski Oct 19, 2016 Contributor

the snippet has been removed, it was to copy a model instance using the Django _meta:

def copy_model_instance(obj):
    """
    https://djangosnippets.org/snippets/1040/
    """
    return {f.name: getattr(obj, f.name) for f in obj._meta.fields
            if not isinstance(f, AutoField) and
            f not in obj._meta.parents.values()}

But Django formalized and refactored the _meta Options API starting with Django-1.8, so this snippet was probably removed since it is now obsolete.

Django recommends using get_fields() in the new Options API and discourages using _fields as indicated in the snippet. Therefore I replaced the snippet with [f for f in get_fields(include_parents=False) if not f.auto_created] but I didn't remove the link.

For more background on the new _meta Options API see this summer of code refactoring project on the django wiki, Django ticket 12663 and Django PR 3114.

@mikofski
mikofski Oct 19, 2016 Contributor

I removed the outdated link to the snippet in 0b91a5a but forgot to change the commit message, sorry!

@@ -58,8 +60,10 @@ def load_data(self, *args, **kwargs):
"""
# get positional argument names from parameters and apply them to args
# update data with additional kwargs
@anomam
anomam Oct 19, 2016 Contributor

There's a lot of context I need to catch-up on here...

@@ -171,6 +180,15 @@ def __init__(self):
else:
#: parameter file
self.param_file = None
+ # XXX: Hack to get PR#68 done
+ if not self.parameters:
@anomam
anomam Oct 27, 2016 edited Contributor

After reading the PR 10 times and Carousel code (+ some blogs on metaclasses and __new__()) I'm starting to get a small understanding of things.

I thought that mcs.set_param_file_or_parameters() and the few lines above would guarantee that self.parameters would never be None? Or is it in case the user does not specify any parameter?
Does it also check for missing fields? For instance if the user forget to specify 'frequency'.

@anomam
anomam Oct 27, 2016 Contributor

Ok I think I understand something: the meta parameters like 'frequency' will always be present in the registry, but they will not necessarily apply to all parameters.

@anomam
Contributor
anomam commented Oct 27, 2016 edited

I get an error when trying to run the pvpower model:

in sandia_perfmod_newstyle.py

----> 5 from carousel.core.data_sources import DataSource, DataParameter
      6 from carousel.core.formulas import Formula, FormulaParameter
      7 from carousel.core.calculations import Calc

ImportError: cannot import name DataParameter
@anomam
Contributor
anomam commented Oct 27, 2016

Hey @mikofski, after spending a while on it I think I understand some of the big lines here, which is good. But a big portion of the commits is still quite abstract for me, so even if I can read the code and understand it line by line, I don't necessarily understand the context and the thought process.
I do think though that I reached a point where I understand the global context enough to be able to talk about some of the implementations, and for this reason I'll switch to some of the model implementation.

Apart from the issue with the pvpower example, the PR looks ok to me.

@mikofski mikofski remove comment with link to Django snippet 1040
* the snippet was removed since it's now obsolete - the new Django Meta
 o\Options API has new methods, _eg_: `get_fields()`, to get fields, to
 filter out parent, autogenerated and reverse fields from a model
* add new comment that links to new Django Meta Options API
f9b396e
@mikofski mikofski merged commit 4ffe1ec into SunPower:master Oct 31, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mikofski mikofski deleted the mikofski:fields branch Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment