-
Notifications
You must be signed in to change notification settings - Fork 8
Rewrote Emissions model for configurable emission methods and options #56
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
Conversation
|
Fixes #35 |
ian-ross
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of general comments, that go not just for this PR, but for most of the AEIC code:
- The top-level organization of the code is odd. We have an
AEICdirectory (which containsperformance_model.pyandtrajectories), anemissionsdirectory, amissionsdirectory, etc. I guess the main question here is "What is theAEICdirectory for?" Should the things in that directory just be at the top level? - There are "loose data types" everywhere. You can make everything much more readable and traceable by using more explicit data types. Instead of a dictionary, use a dataclass. (In a lot of cases, I would consider seeing a dictionary access to be a code smell.) Instead of a tuple, use a dataclass. (Even if it's only a pair: in a
tuple[float, float]for a geographical location, isx[0]the longitude or the latitude? Opinions vary...) First example: all the configuration data is in dictionaries. If you use dataclasses instead, VS Code (or whatever editor you use) can tell you when you get the name of an attribute wrong before you run the code. Second example (related to this PR): all the EI functions return anonymous tuples. The..._EINOxfunctions have returns types oftuple[X], whereXis 6 (six) repeats ofnp.ndarray. Quick! What's the fourth entry there mean? I bet no-one can remember without looking. Give things names. It makes everything easier to understand.
As for this PR, I think there is one main point of design that I would change. The emissions calculation fundamentally seems as though it's a function: you take a performance model and a trajectory and calculate a set of emissions. That result (the set of emissions) is not represented anywhere explicitly in this code — there is no entity called EmissionsData: the Emission class is actually a sort of functional wrapper that does the emissions calculation. None of the methods that do emissions calculations actually return any values. They just modify the Emission object for which they're called, leaving results in attributes that a user of the Emission class has to know to look at.
I think it would be much more usable to have an EmissionsCalculator class that's initialized with whatever configuration data it needs, and that has an emit method with a signature like emit(performance_model: PerformanceModel, traj: Trajectory) -> Emissions that explicitly returns the emissions data. If you want to be able to return just the LTO emissions, just the APU emissions or whatever, then break that Emissions class down into LTOEmissions, APUEmissions, etc., and have emit_lto, emit_apu, etc. methods with the appropriate signatures, and have the overall emit method call those things and put together the full emissions data output.
The reasons for this? First, follow the general principle of "make things real". If you have a concept (in this case a set of emissions data), give it a name, make it into a class that you can instantiate, and see what that makes your code look like. It almost always makes it more readable and more usable. Second, it makes more sense to me to have an EmissionsCalculator (a lot like the trajectory builder classes) that is reusable for multiple trajectories, since the most common use case we'll have will be running this thing over a bunch of trajectories to add emissions data to them.
(Some of the signatures and organization will change as we flesh out how the missions, trajectories and emissions code work together, but I think the principle of having a "functional" emissions calculator will always hold.)
| def __emission_dtype(self, shape): | ||
| """Build the structured dtype used for every emission array.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't like this approach very much! We've talked about this one before though, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still have this as I don't know the solution for this yet - happy to discuss next meeting
|
@ian-ross Incorporated all your feedback - let me know if it looks ok now. I've added docs too so should have info on what emisisons looks like now |
…ions not in config set to -1
…and traj module so rewrote all of it
469f8ea to
be6b172
Compare
|
@ian-ross I've updated this branch with all the latest updates - I think it's ready for review |
ian-ross
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally sold on all of this, but this version is definitely better than the first one. I think the best way to proceed is for us to merge this, then we'll rework it piece by piece. I have some work in progress to improve a lot of the configuration data handling and the way that performance models are built and used, and some of that will be relevant to this.
So, approved. Go ahead and merge!
Added functionality to choose which emissions to compute via the configuration file
For emissions dependent on just fuel EI, added bool type option of computing
For emissions that have more variables, added options for different methods of computation (For eg: BFFM2 and P3T3 for NOx)
Rewrote emissions class to add this functionality.
Also rewrote emissions tests - not only because of changes in original class but also the old tests depended on the correct functionality of performanceModel and Trajectory modules which is not how the tests should work. If there was an error in the other two modules it would cause an error in Emissions testing making it hard to find where the error really is.
@ian-ross I tried a couple methods and this seemed to work best. It was a bit messy at first but I've tried to clean it up and make it look elegant. Let me know if you think there's better ways of doing this.