ICF Off-model calculator - Refactor 1-3, Feature 1-3 #74
Conversation
…improve code structure.
… calculator functions
|
Hello - Thanks for making this PR! Appreciate it! I'll drop some initial comments (nothing huge) as comments. |
There was a problem hiding this comment.
I think these files should live on Box rather than in GitHub. It's likely our workflow will include a number of iterations on model runs and there's no need to repeatedly commit data files to GitHub since they don't serve any purpose here, and Box keeps versions/timestamps.
There was a problem hiding this comment.
@lmz I agree, the purpose of these files (at least having a couple of them as samples) is to give flexibility to people who don't have access to BOX to run the whole script as a demo. This is helpful for me or people on our team. In this way, anyone can demo the script and provide improvements later. The end goal is that for all MTC team, they'll automatically use the paths in the mtc.py file which point directly to BOX. Additionally, it's a way to clearly state as a sample the shape in which we are expecting to receive the inputs.
There was a problem hiding this comment.
It's trivial to give other folks access to Box; please request if you need others to have access. We generally keep data files on Box and not on GitHub, and I'd prefer to keep it that way; having it both ways adds confusion.
There was a problem hiding this comment.
This folder (templates) has three files which makes you click around and the paths could just be done using an if statement without this kind of complexity. A simple if would make it clear what variables need to be defined for each environment and this structure is more error prone. Suggest deprecating this, and just inlining this logic somewhere else that's logical.
Also, please use pathlib for paths rather than strings with replace("\\","/"). pathlib handles forward versus backward slashes, etc and it is much more readable.
There was a problem hiding this comment.
@lmz Thanks for the suggestion, I will keep them together instead in multiple locations and propose a new location for this logic.
There was a problem hiding this comment.
Please rename this to be something more descriptive; suggest "OffModelCalculator" rather than "Calc". Also, please add documentation for the class variables and methods.
There was a problem hiding this comment.
Also, let's just make the args that are passed into the constructor explicit, rather than hiding them in "args" -- there's only two...
There was a problem hiding this comment.
@lmz sounds good. I've integrated this change in a new commit.
There was a problem hiding this comment.
What does "mons" mean?
If these methods are for the Calc class, can they just be static methods of the class? Ditto for methods in runs.py
There was a problem hiding this comment.
@lmz "mons" was a shorter name for commons. I will rename the file as commons.py. Here I want to add helper methods that are common between multiple calculators were transformations are needed.
I can definitely keep the methods in "commons" and "runs" in the same file. However, I wanted to have the OffModelCalculator class to include only the methods that are necessary in the high-level process. In this way, I can follow the update process in a more clear-cut way. Would that be okay with you?
There was a problem hiding this comment.
The terminology is confusing here... what a "NewRun"? Internally, we think of model runs being Travel Model Runs, but we wouldn't be creating a new run here. Suggest some clarity -- if not in the method naming, through method documentation.
There was a problem hiding this comment.
@lmz sounds good! I've added the method documentation.
There was a problem hiding this comment.
Similarly -- what is this? I would think this should go on Box?
There was a problem hiding this comment.
@lmz the IPA_TM2 folder saved in the inputs is the folder we received as an example of what the R script created (previous step). I am not touching this folder design because I'm not updating the R script. I'm only using it as the expected input sample folder.
Once we complete all the updates and run a demo, we can remove all the files that are unnecessary in the process, if applicable. My understanding is that we only need the files in IPA_TM2/ModelData. Ideally, we'd like the R script to reflect that.
There was a problem hiding this comment.
What is this? Does it need to be excel? It makes it hard to see diffs, so if it could be a CSV instead, that would be preferrable.
There was a problem hiding this comment.
@lmz this file was created with the intention that the Off Model Calculator manager could update the Calculators in the folder "models" and then, update the Variable_locations Excel file with the added/updated cell locations. We're using excel as a UI to make it easier to update. The end goal is for the manager to update the calculators and be able to run the script even when cells have changed location in the individual calculators.
Now, the Off Model Calculators latest version will be in the folder "models" to be used as the source of truth. Those files will keep a log of the runs that have been executed.
There was a problem hiding this comment.
Rather than commenting out a bunch of code, go ahead and just remove it, since it's still viewable in the git history...
There was a problem hiding this comment.
@lmz absolutely! I am still in the process of converting the missing calculators, so I wanted to see each update process. Once I complete this step, I will delete all the commented code and keep it clean.
|
@lmz Thanks a lot for all the feedback. I will start working on this and respond/add new commits. I appreciate all the detailed responses. |
…model-one into icfOffmodelExcelCalc
caroarriaga
changed the title
attempting to capture travel time on no truck lanes on I580/SR85 with USE==4
Reverting change
…/travel-model-one into icfOffmodelExcelCalc
|
Hi @caroarriaga & @yuqiww - The branch is current master plus the commits from https://github.com/caroarriaga/travel-model-one/tree/icfOffmodelExcelCalc merged in. You can see the diffs with master in this PR: #76 @yuqiww: It's cloned to Let me know if you have issues! |
|
Hi @lmz , I've sync'd the upstream repo and my forked version. I also went over the new branch you created to collaborate. I will create another PR to the new branch you created (to push there instead of master). Let me know if you'd prefer I handle future collaboration differently. |
|
Sounds good, @caroarriaga ! Thank you! |
Summary
This PR refactors the architecture of the script processing the copying and updating off-model calculator files.
Additionally, it includes the first part of Feature 1 applied to the bike share calculator.
Details
Refactor 1: rearranged methods into calculator classes and subclasses.
Refactor 2: added user type (mtc, external) to point to specific directory paths.
Refactor 3: the calculator updates SB375 data into all calculators.
Feature 1: all calculator variables and corresponding cell location in Excel are centralized.
Feature 2: automatically open and close excel to update formulas.
Feature 3: log in the corresponding calculators the outputs.