Making hillmaker object oriented

[misken]

I've thought about this in the past and @jwnorm brought it up as well. We can use this discussion as a place to bat around ideas about the OO structure. For example, to start, it could be as simple as having a hills class that contains results related things and maybe a hillmaker class that could be thought of as a single scenario, or perhaps, an umbrella class that might contain zero or more scenario objects and corresponding hills objects? I really haven't thought very much about this (obviously) and am open to ideas.

[jwnorm]

I like your idea of a Scenario subclass. With that, I am imagining it could look something like this:

Hills Attributes

• stops_df
• in_field / out field
• output_path (optional)
• Scenario subclass
  • scenario
  • start_analysis_dt
  • end_analysis_dt

Scenario Class Methods

• make_hills(self, bin_size_minutes=60, export=False)
• make_plot(self, metric, day_of_week='full', capacity=None, export=False)
• get_summary_stats(self, threshold=1) --> infer hours of operation and other statistics from output of make_hills.
• etc...

An example piece of code would be:

hill = Hills('ShortStay.csv', 'InRoomTS', 'OutRoomTS', './output')
scenario1 = hill.Scenario('My First Scenario', '1996-01-01'. '1997-01-01')
hill_summary1 = scenario1.make_hills(export=True)
hill_occ_plot = scenario1.make_plot('occupancy', day_of_week='Tue', capacity=20)

In my view, the goal should be to make it easy for users to run multiple scenarios on the same data set. One bonus would be that the arguments to the make_hills function would drastically shrink. It would run faster as well since 20+ plots are not generated at runtime, only on the users explicit request for specific ones. I also think users would be apply to apply custom formatting and styles to their plots since they can work with hillmaker programmatically.

[jwnorm]

baby steps to oo hillmaker

From: @misken

I got the most basic of OO refactoring of hillmaker done. I'm attaching a notebook (and exported html version) that shows the basic usage from code. The code is in the branch named oo on GH. Rough summary of what I did:

• created a HillsScenario class (hills.py) that has two attributes:

  • scenario_params - a Parameters object (described next) containing all the input parameters,
  • hills - dictionary containing all the outputs, including plots

• When you instantiate a HillsScenario object it's got flexibility in that kwargs can be used or a toml config file can be specified or both with kwargs getting priority over the toml config specs.

• created a Parameters class (parameters.py) that is a Pydantic model that handles most of the input validation.

• when using from code, have separate options to generate week plot and dow plot and separate options to export them to png

• HillsScenario has a make_hills() method that just wraps the existing make_hills() func.

One annoyance for now is that the make_hills() function is taking a validated Parameters object as its only input arg and thus we don't have autocomplete or function help. I'll figure out a work around.

Let me know what you think. For now I tried to keep all the same parameters to minimize code changes. I just tried to focus on a basic OO framework.

[jwnorm]

This looks really good! Being able to save a scenario instance will be a huge time saver when working programmatically.

When you are in a place where you are ready to make changes that may break older code that uses hillmaker, here are some potential enhancements that come to mind:

• Consider splitting out the plotting functionality from the make_hills method when used in an OO context. For example:

scenario_1 = hm.HillsScenario(…)
scenario_1.make_hills()
scenario_1.generate_plot(“occupancy”, day_of_week=”full”)

• Accessing the DataFrames via several dictionary keys sometimes feels clunky. I remember when I first started using hillmaker I had to dive into the source code to understand what was going on. Maybe a getter method to access a df would be better? They can still be stored in dictionaries, but to the user they would be accessible like so (using the same example as above):

occ = scenario_1.get_df(“occupancy”, stationary=False)
arr = scenario_1.get_df(“arrival”, stationary=False)

• Continuing in this same vein, I imagine once there is a LOS summary (and potentially more summary types), you could access them in a similar fashion. Not sure if these would be better off being actually computed when the method is run, or if they are getters as well:

los_summary = scenario_1.summarize(“los”)
hours_summary = scenario_1.summarize(“hours_of_operation”)

These ideas all come from my personal use of hillmaker along with experience using similar(ish) libraries. Of course, I still think it is very important for make_hills to be able to run as a single function using a CLI/GUI that is fed a toml (or directly given parameters with kwargs). Maybe the OO method make_hills and the global function make_hills eventually have significant differences, since the OO organization lends itself to “a la carte” computing vs all or nothing.

Let me know if you need me to expand my thoughts on anything.

[jwnorm]

From: @misken

See below.

• Consider splitting out the plotting functionality from the make_hills method when used in an OO context. For example:

scenario_1 = hm.HillsScenario(…)
scenario_1.make_hills()
scenario_1.generate_plot(“occupancy”, day_of_week=”full”)

I'm struggling with this basic issue. I do think that the simplest default case of calling make_hills() method should generate the plots. The plots really are the "hills" and the default use case would be to generate plots. So, that's why right now it's implemented that to do the above, you'd have to call make_hills and use the option to suppress the hill plots. Maybe there should be a third method that is compute_stats() that does not generate plots and make_hills() is basically the same as compute_stats() followed by generate_plot().

• Accessing the DataFrames via several dictionary keys sometimes feels clunky. I remember when I first started using hillmaker I had to dive into the source code to understand what was going on. Maybe a getter method to access a df would be better? They can still be stored in dictionaries, but to the user they would be accessible like so (using the same example as above):

occ = scenario_1.get_df(“occupancy”, stationary=False)
arr = scenario_1.get_df(“arrival”, stationary=False)

Totally agree. I like the getter method idea.

• Continuing in this same vein, I imagine once there is a LOS summary (and potentially more summary types), you could access them in a similar fashion. Not sure if these would be better off being actually computed when the method is run, or if they are getters as well:

los_summary = scenario_1.summarize(“los”)
hours_summary = scenario_1.summarize(“hours_of_operation”)

Yep, agree.

These ideas all come from my personal use of hillmaker along with experience using similar(ish) libraries. Of course, I still think it is very important for make_hills to be able to run as a single function using a CLI/GUI that is fed a toml (or directly given parameters with kwargs). Maybe the OO method make_hills and the global function make_hills eventually have significant differences, since the OO organization lends itself to “a la carte” computing vs all or nothing.

Yes, this is one of the main interface design decisions that we need to think through.

[jwnorm]

I see what you are saying with the make_hills function literally making the hills (plots). That makes a lot of sense to me and is more in line with the current prod version of hillmaker.

I think a compute_stats function that generates the summary dfs is a great way to allow for customization in Jupyter notebooks and scripts. This also gets to your idea that it could be executed faster since the user would be excluding certain exports.

This also keeps the OO method make_hills in line with the function make_hills.