Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
matmair opened this issue Jun 30, 2022 · 7 comments

Comments

@matmair
Copy link

matmair commented Jun 30, 2022

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
Copy link
Owner

timcera commented Jul 2, 2022

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

@matmair matmair changed the title [NUG] Nodes with names consisting of numbers and single underscores are considered as numbers [BUG] Nodes with names consisting of numbers and single underscores are considered as numbers Jul 2, 2022
@matmair
Copy link
Author

matmair commented Jul 2, 2022

@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
Copy link
Owner

timcera commented Jul 24, 2023

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

@lucashtnguyen
Copy link

@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 tsutils is used for outside of swmmtoolbox

@timcera
Copy link
Owner

timcera commented Jun 12, 2024

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
Copy link
Owner

timcera commented Jun 13, 2024

@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
Copy link

sanjaypatel-confluency commented Jun 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants