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

punionWith flips arguments #556

Open
facundominguez opened this issue Aug 22, 2022 · 5 comments · May be fixed by #558
Open

punionWith flips arguments #556

facundominguez opened this issue Aug 22, 2022 · 5 comments · May be fixed by #558
Assignees
Labels
bug Something isn't working

Comments

@facundominguez
Copy link
Contributor

facundominguez commented Aug 22, 2022

I found the following inputs to punionWith which seem to flip the order of the arguments.

firstPValue:
  [ (0x,[(0x,3000000)])
  , (0x5ad56f5..., [(0xfb8c7da5...,1)])
  , (0xa912171...,
      [ (0x676f6c6...,24)
      , (0x73696c7.,91)
      ]
    )
  ]

secondPValue:
  [(0xa912171..., [(0x676f6c6...,1)])]

punionWith (-) firstPValue secondPValue:
  [ (0x,[(0x,3000000)])
  , (0x5ad56f5..., [(0xfb8c7da5...,1)])
  , (0xa912171...,
      [ (0x676f6c6...,-23)
      , (0x73696c7...,91)
      ]
    )
  ]

It turns out that punionWith (-), ends up calling this code which does appear to flip arguments:

# (mergeInsert # y # ys' # xs)

The fix could be to declare that punionWith is to be used with commutative operations only, or it could be that punionWith needs to stop flipping arguments. In any case, I'd be thankful for any hints on how punionWith is expected to work.

@facundominguez facundominguez changed the title psubtractValue flips arguments punionWith flips arguments Aug 22, 2022
@L-as
Copy link
Member

L-as commented Aug 22, 2022

This is horrible @blamario

@L-as
Copy link
Member

L-as commented Aug 22, 2022

I'm not sure the code will still work if you change that snippet, but we could keep track of flipping to flip application to the combine function if necessary.

@blamario blamario self-assigned this Aug 23, 2022
@blamario
Copy link
Collaborator

Yup it's bad, sorry. I wouldn't just flip there because of accumulating flip applications.

@L-as
Copy link
Member

L-as commented Aug 23, 2022

Brian Kush just noted that (obviously) punionWith would never work with -, because if you punionWith (-) with two disjoint values, you get the sum necessarily.
punionWith needs to be completely redesigned. There should be two extra functions for handling the two extra cases where the LHS or RHS doesn't exist in the most efficient way possible.

@blamario

@blamario
Copy link
Collaborator

Are these two extra functions necessary? I'm not sure punionWithDefault (-) 0 a b would be any more efficient or convenient than punionWith (+) a (pinv b). Are there any other use cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants