search

pyswmm / Stormwater-Management-Model

PySWMM Stormwater Management Model repository
Other
99 stars 77 forks source link

Test merge usepa522 #378

Closed bemcdonnell closed 1 year ago

bemcdonnell commented 1 year ago

Closes #377

bemcdonnell commented 1 year ago

Currently checking where CI fails. I am aware this push will fail the build somewhere. Will make some quick edits to error handling in the next commit

bemcdonnell commented 1 year ago

Awesome. Code builds are working and tests are failing. Will go through and clean up the testing assertions and LID numerical parts. (FYI @abhiramm7, @cbuahin, @karosc). I will be brining you in for review ASAP. There are many changes but we can isolate them to a small set:

  1. some new API parts. I am cherry picking only things that are compatible with the OWA API.. I had to remove many tests since there are no unit tests for them. The implementations have conflicting behaviors.
  2. the error.h/.c sections have been completely overhauled. I like how it is done today.. No longer do we have to deal with the enum index. Good approach now
  3. the new calculation feature enhancements that Lew made in releases through 5.2.2.

I have merged the codebases and I've taken a few passes through. I'm generally pleased with the build... But, as I said, I am going to noodle my way through fixing the test assertion errors. I will probably even add a bundle of unit tests to help the effort.

Shout out to all of you who have done significant development on the build process! It's been a while since I've touched this code but I'm incredibly proud to be a developer among this team. You're all great!

abhiramm7 commented 1 year ago

This is awesome 🤩 @bemcdonnell

cbuahin commented 1 year ago

Does this mean we are skipping the intermediate versions?

abhiramm7 commented 1 year ago

@cbuahin good question! Do we need the older versions? This version would include the updates from the previous updates. I guess we need the older versions for posterity sake.

dickinsonre commented 1 year ago

I would use only 5.2.2 as it includes all of the improvements of 5.2 and 5.2.1 but with bugs introduced in 5.2.2 fixed. I doubt you need 5.2 as it has a a few control rule bugs.

abhiramm7 commented 1 year ago

@dickinsonre, that's a good point.

It is not as straightforward as I thought. We might need to talk more about how we will approach the update. Unless, @bemcdonnell, you feel we need this update for some feature you are considering for pyswmm.

bemcdonnell commented 1 year ago

@abhiramm7, I’m just trying to be realistic with our time. I don’t think it’s critical we do all the point releases anymore. Once we are caught up, we can do a better job keeping it up to date. :-)