impactlab / eemeter

‼️ MOVED TO https://github.com/openeemeter/eemeter - Core computation engine for the Open Energy Efficiency Meter
https://eemeter.readthedocs.io/
MIT License
25 stars 13 forks source link

Refactor GreenButton parser #119

Closed jpvelez closed 8 years ago

jpvelez commented 8 years ago

So, this entire refactor worked after squashing too tiny bugs, so I'm extremely paranoid. Please get this a thorough scrub, @philngo. I'd also appreciate your help testing it tomorrow/Tuesday.

mcwp commented 8 years ago

@jpvelez : I thought I would test your refactor with my learning code, and to see if my hacks might no longer be needed. (Workaround duplicate 15 minute interval in early November each year; get the fuel type out of the xml instead of hard coding it to electricity; converting my electric consumption data from 15 minute to 1 day intervals before importing.)

However, I see that your refactor is of parser.py, not importer.py, and there is a comment in importer.py about deprecating and using parser.py instead. Wish I'd seen that sooner! Will GreenButtonParser be updated to be created from a file instead of a string? Then I could replace this line with these two lines instead:

    gb_g = GreenButtonParser(gbutton_g)
    cd_g = gb_g.get_consumption_data_objects()  # expect list of one object

I tried to hack around this but now I run into this error:

ValueError: empty namespace prefix is not supported in ElementPath

while trying to get the timezone from my data. That is probably because of the difference between the root element in your string data vs. the root element in my file data (pge Opower).

(Pdb) p self.root.nsmap
{'ns0': 'http://naesb.org/espi', 'ns1': 'http://www.w3.org/2005/Atom'}

(Pdb) gb_e.root.nsmap
{'xsi': 'http://www.w3.org/2001/XMLSchema-instance', None: 'http://www.w3.org/2005/Atom'}

So I'll wait until your tests are passing and either the parser takes a file or the importer is updated to use the parser. Then if there is still that namespace error, and the parser is supposed to work with greenbutton data from pge, then I'll file a bug.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 0.0% when pulling 6addcc672314ea9298dd83591e26e102a679f70e on parser-refactor into 6fad2214ff9a774fa3d904b5a2cc75cd09d5be1e on master.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 0.0% when pulling 370631f18d716434b555380c6dd11072aa28d509 on parser-refactor into 6fad2214ff9a774fa3d904b5a2cc75cd09d5be1e on master.

philngo commented 8 years ago

@jpvelez, I went through the code and made some minor adjustments, including various updates that fixed up the tests.

I removed all the nsmap stuff as well and went back to namespaces explicitly, as you had originally done. @mcwp That should fix the mismatched namespace error you were seeing. Per your request, I added the ability to read in files by filename so that you can directly swap in the new parser. This is the new one I've referred to once or twice that only existed since last week, which is around when I added the little deprecated comment. You didn't see it because it didn't exist!

In a moment I'll push another commit that adds some docs - once I confirm the docs are building correctly and the broken init test above is fixed, I'll merge.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 0.0% when pulling 49fbdb0cb93d250a631eaa480c34a7d01e5877d8 on parser-refactor into 6fad2214ff9a774fa3d904b5a2cc75cd09d5be1e on master.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 0.0% when pulling 49fbdb0cb93d250a631eaa480c34a7d01e5877d8 on parser-refactor into 6fad2214ff9a774fa3d904b5a2cc75cd09d5be1e on master.