search

timcera / swmmtoolbox

Command line script and Python package to read time-series from the Storm Water Management Model (SWMM) binary output.
BSD 3-Clause "New" or "Revised" License
17 stars 7 forks source link

[BUG] Nodes with names consisting of numbers and single underscores are considered as numbers #12

Open matmair opened 2 years ago

matmair commented 2 years ago

Hi there @timcera ! Thank you for this great software. I ran into the following problem today: 'node,19_3,Depth_above_invert' leads to the node label beeing interpreted as 193 in acordance to https://peps.python.org/pep-0515/. The issues seems to be part of tsutils.make_list. I will leave a comment with a solution if I find any.

timcera commented 2 years ago

Could you post the output file with node "19_3"? Just a short run.

matmair commented 2 years ago

@timcera I will create a sample snippet and post the file and the run here once I have access to the data on Monday again.

timcera commented 1 year ago

I will take a look at this. I can include some logic to not convert to float or int for swmmtoolbox.

lucashtnguyen commented 3 months ago

@timcera I'm also running into this issue. I've confirmed that the bug is in tsutils.make_list. Do you have availability / intent to patch this yourself or would you be okay with a PR? Note I don't have good context for what tsutilsis used for outside of swmmtoolbox

timcera commented 3 months ago

A PR would be great!

First choice would be to remove using tsutils.make_list from swmmtoolbox. tsutils.make_list is way overkill, because all swmmtoolbox needs is consistent split of strings on " " and "," if a string is passed. I looked a little bit at this option and it is a bit painful because it needs to support both Python and command line APIs.

A close second choice would be to move processing of labels for the command line API to extract_cli, and then remove all labels processing from the extract Python API which could then expect labels to be a Python list of lists. Maybe this idea should be first actually.

Third choice would be to add a keyword to tsutils.make_list, something like "return_all_strings=False" as the default, which if True wouldn't convert number like strings to numbers.

Any help would be appreciated. Just so there isn't duplication of effort, I might be able to work on this next week and I am going to explore the second choice because that might help with some of my other toolboxes. However, any help is appreciated and if you have some idea or workaround that you can put together into a PR that solves your problem that would be fantastic.

timcera commented 3 months ago

@matmair and @lucashtnguyen could you try 4.0.14 that I posted to pypi last night? Should be available from conda-forge in a little bit.

I implemented the second choice from above. I now enforce in the Python API that labels must be a list of list like [["link", "222", "Flow_rate"], ["node", "222", "Hydraulic_head"]] where you used to be able to pass a single string "link,222,Flow_rate node,222,Hydraulic_head" similar to what is read from the command line interface. I think this new approach is appropriate because if you are using the Python API you should use Python structures also.

I don't have a test for this bug and I wondered if either of you can send me a small output file with a node or link name that would trigger this problem.

sanjaypatel-confluency commented 3 months ago

Hi @timcera,

thanks for coordinating with @lucashtnguyen and making this fix.

4.0.14 works for our needs Attached is a test file that i tried it on. It is an .OUT file but i've had to rename it

this functions in 4.0.14 swmmtoolbox.extract(outfile_location, [['node', '6666_8', 'Total_inflow']])

but was failing in 4.0.13 (different calling syntax) swmmtoolbox.extract(outfile_location, 'node, 6666_8, Total_inflow')

Thanks Sanjay zeta_renamed_tank.out.rename.zip