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

FinFunction does not constrain assignments to be in codomain #751

Closed
corybrunson opened this issue Mar 19, 2023 · 2 comments · Fixed by #759
Closed

FinFunction does not constrain assignments to be in codomain #751

corybrunson opened this issue Mar 19, 2023 · 2 comments · Fixed by #759

Comments

@corybrunson
Copy link

I'm experimenting with Catlab and wondering why i don't get an error with the following:

n = FinSet(4)
m = FinSet(2)
f = FinFunction([3,2,3,1], n, m)

I'm able to pretty_table() the result, and it shows that 1, 2, 3, and 4 are mapped to 3, 2, 3, and 1, respectively; but what does this mean when 3 is not in m?

This seems like it could cause unexpected composition errors, like this:

o = FinSet(1)
g = FinFunction([1, 1], m, o)
fg = compose(f, g)
#> BoundsError: attempt to access 2-element Vector{Int64} at index [[1, 2, 3, 3]]

Thank you for any help!

@lukem12345
Copy link
Member

After asking around, the consensus is essentially that there are some engineering trade-offs in determining when to check whether a function is well-defined. Here, the trade-off is having fast construction of FinFunctions over explicitly checking at construction-time whether the provided codomain fits.

As you note, this mismatch is indeed caught at composition time.

The current behavior might be improved by:

  1. Providing a function that checks that this condition is met.
  2. Check at construction that this condition is met, unless some flag is set, like what happens when you use the @inbounds hint.

These are some suggestions raised by @epatters and paraphrased by me.

@corybrunson
Copy link
Author

Thank you! While i'm not a power-user of AJ, my impression is that (1) would not be much help, as most users would not deviate from default settings in order to perform such checks.

Having a flag à la (2) that could be set globally (for a session) makes sense to me, but perhaps not if this is the only constructor for which the check would be relevant. Is FinFunction one of a larger class of constructors with this behavior?

I would still defer to y'all on which, if any, change is best.

@epatters epatters changed the title codomain of FinFunction does not constrain image assignments FinFunction does not constrain assignments to be in codomain Apr 2, 2023
@KevinDCarlson KevinDCarlson self-assigned this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants