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

Cannot copy comparer collection #423

Closed
daniel-caichac-DHI opened this issue Mar 1, 2024 · 6 comments · Fixed by #424
Closed

Cannot copy comparer collection #423

daniel-caichac-DHI opened this issue Mar 1, 2024 · 6 comments · Fixed by #424

Comments

@daniel-caichac-DHI
Copy link
Collaborator

Hi, I think this is a bug (?)
If I have a compare collection, I cannot make a copy of it :/ , but I can copy a single comparer.
Is this ok @ecomodeller ??
image

@jsmariegaard
Copy link
Member

Looks like a bug 🤔

@ecomodeller
Copy link
Member

Looks like a bug 🤔

It does.

@daniel-caichac-DHI in order to fix this properly, can you explain why you want to copy the ComparerCollection in the first place?

@daniel-caichac-DHI
Copy link
Collaborator Author

Kind of legacy code now, I might need to update it, but I was accidentally replacing the data of a comprar collection in one of my wrappers, so then I said 'of course, I first need to make a copy of the comparer collector , then I am not modifying my input anymore", and bum, crash.

As of now I added a try/expect in my wrappers, so not urgent, but thought you might want to know

@jsmariegaard
Copy link
Member

I think it is sensible to copy a whole collection if you want to make changes to the data. It should not be necessary for filtering and analysis.

@daniel-caichac-DHI
Copy link
Collaborator Author

daniel-caichac-DHI commented Mar 1, 2024

I had my wrappers implemented before the .query and so, but some day I should refactor my code to the new modelskill filtering implementation. Still, the .copy() should work, I reckon

@jsmariegaard
Copy link
Member

I guess Henrik is considering whether it should be removed rather than fixed...

@ecomodeller ecomodeller linked a pull request Mar 1, 2024 that will close this issue
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

Successfully merging a pull request may close this issue.

3 participants