Update penalty code

[Amasus]

I'm sitting down to review the pentaly update reivew, which is good, and I'm summarizing my thoughts here.

The Good

• Both the tests/ folder and a Gwinett original config and the tests passed / output matched
• The penalized yaml is very clear and helps me understand what is going on
• I see that @abd1tus's concerns about logging has been addressed, which is great
• You guys have done a ton of work and I want to appreciate that up front. THANK YOU.

The Bad

• @dxwils3, There is no new testing written to make certain that I don't accidentally do something to break the penalty functions
  • I lost several weeks of project time to someone trying to be helpful and changing a single parameter that should not have been changed.
  • I'd rather that not happen to me again, or to anyone else for the first time on this project
• There is no explanation of what is going on with the penalty function in the readme, nor is there any discussion of the new field in the config file in the same.
  • if not documented, anyone usuing our the tool will not know that this feature exists, let alone how to use it.
  • That includes me
• @abd1tus has created a class called PollingModel in model_factory that is there to help identify what parts of model are parameters, and what are variables, etc. None of the penalty parameters (are there also variables?) are added
• There have been significant changes made to run_on_config (in model_run.py) without any documentation
  • model_name: what is this?
  • my_print: what is this? Why are we doing things like this rather than just using standard logging?
  • model_string?????
  • config.name: there isn't a name field in the config file. Is this like config.config_file_path?
• The original intent of model_run was to have a high level file with a few lines of code that was easy to read instruction set of what one does to get the output. As such, I wrote it to have functions called, but not defined. I also wanted minimal logic in the file. That is not how model_run currently reads.

The Next Steps

I'm reverting main back to before the merge for now. @abd1tus , we should talk about what the actual review process / standards we want to hold the code to is before we accept a merge from anyone on the team. It is quite possible that I'm making changes to main that are driving someone else up a wall, but no one is saying something.

For now, given a lack of process, I'm going to ask for the following changes to be made, and then reviewed before a the penalty work is incorporated into main. After the changes have been made,

1. make a merge request,
2. give @abd1tus and @Amasus a chance to review the code
3. set up a time wtih the whole team to walk through the changes that have been made so that we all understand what is being implemented and how to use it.

Please feel free to ask any questions / objections.

Requested changes:

• @dxwils3 @dskipper can you add testing for the penalty work. At the very least, I'm seeing the following new constraints:
  • build_exclude_sites_rule: @dskipper what is this doing/ how is this different from the "drop these from the data frame logic" in model_data
  • build_penalty_rule
  • build_penalty_approximation_rule @dskipper can you fix the doc string for this? I have no idea what this rule is doing
• There is a Gwinnett_config_expanded_penalized_nonaverse_20.yaml file that seems to be identical to Gwinnett_config_expanded_penalized_20.yaml
  • @dskipper can you explain to @Amasus what the averse is doing, or if this is still in progress, just remove it from the files pushed?
  • @dskipper or @dxwils3 can you update the readme to include a discussion of the work that to be incorporated into main?
  • @dxwils3 add the parameters and variables relevant to penalty work to the PollingModel class in model_factory
• @dskipper @dskipper can you add documentation around the penalty work
  • In the read me
  • in model factory, line 286 (starts if site_penalty.) A few words around here reminding the reader what constraints are being implemented and why would be useful
  • a few words about the exclude_penalized_sites flag. It is defaulting to False, and always needs to be included, but when should it be true? How should we think of this?
  • make model_run clean again
  • take all the logic that has been added and either put it into a function in model_solver or create a new python file that has it has a function and call it.
    • nameing schema for relevant python files is model_descriptor.py
  • rename the function my_print with something more reasonable, document.
    • work with @abd1tus on this if need be, as he's been on top of keeping the logging clean
  • document and help me understand what is going on with the model_name, and why it is being called in the first place.

[Amasus]

Current copy of old main is in feature/main_before_reverting

[dxwils3]

Thank you for the thorough review. I can totally appreciate your concerns and think your suggestions are typically good, but some of your expectations may not be reasonable with the move from a monolithic model to an iterative algorithm. However, I don't think there's a useful discussion to be had at this point given the disconnect between the level of ambiguity that exists in how to contribute to this project and the certainty with which you express your vision for how things should be done.

I would appreciate if you could add a CONTRIBUTING.md file to lay out your expectations for contributions to this project. Things that would be great to have specifically addressed:

• structure of the project (please address the items you put above, including where the algorithm lives)
• how to land features, create merge requests, review process
  • I would typically merge into develop and only merge into main for releases, but develop is currently 151 commits behind main
• how to do testing
• given that this isn't python standard logging (i.e. import logging), how exactly does it work, what are the logging levels, how does it work in the context of multiprocessing

I will make some changes but others I will postpone until this has been done.

[abd1tus]

@dxwils3 Thanks. Yes, we've reached the point where we do have to start putting more process into this. In the past simple ad hoc changes had sufficed, but we need to move forward with a bit more process now that we are going to bring more volunteers on.

Agenda items for this week will include the following topics:

• Pushes to main and which strategy to use (e.g. gitlab flow, etc)
• Process around ticket writing (Epics) as part of the contribution process as a way to share with group on general concepts of (does need high levels of detail), and keeping ticks up to date
• Updates to our ongoing weekly meeting agendas, including checkins on active tickets
• Using tickets as a focusing tool for discuss and documenting features rather than scattered among messages on discord and meeting notes
• Time for monthly planning session meetings, or as needed
• Drafting the contents of CONTRIBUTING.md. Something along the lines, but much simpler than this example. Particularly for this group:
  • Which git commit strategy to use - including cleaning up or deprecating develop
  • Guidelines on testing and logging practices
  • And which style guide to follow - e.g. Google Python Style Guide.

The major goal of the above is around effecting process to better align contributors while remaining as lean as possible/practical for a small group.

@dxwils3, can you think of anything else that should be added, and do you have any particular favorite strategies or guides?