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.

Sign up for GitHub

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

Jump to bottom

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
@blamario
Labels
bug Something isn't working

Comments

@facundominguez
Copy link
Contributor

facundominguez commented Aug 22, 2022 •
edited

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:

plutarch-plutus/Plutarch/Api/V1/AssocMap.hs

Line 395 in 77bbe60

# (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?

blamario added a commit that referenced this issue Aug 23, 2022
@blamario
Fix for punionWith argument-flipping, issue #556
c1ed4b9
@L-as L-as linked a pull request Oct 10, 2022 that will close this issue
Fix for punionWith argument-flipping, issue #556 #558
Draft
@blamario blamario mentioned this issue Nov 30, 2022
Closed
@SeungheonOh SeungheonOh added the bug Something isn't working label Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@blamario blamario

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix for punionWith argument-flipping, issue #556 Plutonomicon/plutarch-plutus
4 participants
@facundominguez @blamario @L-as @SeungheonOh