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

set_subparts! behavior #53

Open
slwu89 opened this issue Sep 11, 2023 · 6 comments
Open

set_subparts! behavior #53

slwu89 opened this issue Sep 11, 2023 · 6 comments
Labels
question Further information is requested

Comments

@slwu89
Copy link
Member

slwu89 commented Sep 11, 2023

The methods for set_subpart! allow a user ignore the parts argument when it's assumed that the vector vals is the same as the number of parts of name, in this case it dispatches to the method where ::Colon is the 2nd argument.

set_subparts! does not seem to have this behavior, see ex below. Is this intended? If not, I'd be happy to get a simple PR to implement it (and also tests for set_subparts!).

using Catlab

@present SimpleSch(FreeSchema) begin
   X::Ob
   Attr::AttrType
   attr1::Attr(X,Attr)
   attr2::Attr(X,Attr)
   attr3::Attr(X,Attr)
end

@acset_type SimpleData(SimpleSch)

mydata = @acset SimpleData{String} begin
   X=5
end

attr1 = string.(1:5)
attr2 = string.(('A':'Z')[1:5])
attr3 = ["hi","bye","hello","como estas","zai jian"]

# doesn't work
set_subparts!(
   mydata,
   attr1=attr1, attr2=attr2, attr3=attr3
)

# works (it would be nice to be able to do the previous)
set_subparts!(
   mydata,
   :,
   attr1=attr1, attr2=attr2, attr3=attr3
)
@slwu89 slwu89 added the question Further information is requested label Sep 11, 2023
@slwu89
Copy link
Member Author

slwu89 commented Sep 15, 2023

Adding the method @inline set_subparts!(acs; kw...) = set_subparts!(acs, :, (;kw...)) makes the above example work. However, it also means that we can assign subparts that are foreign keys/attributes of different parts, as long as the values are the correct length, see the following example. I am not sure if this is desired behavior, but if so, happy to open a simple PR.

using ACSets

SchAttrs2 = BasicSchema([:X,:Y], [], [:Attr], [(:attr1,:X,:Attr),(:attr2,:X,:Attr),(:attr3,:Y,:Attr),(:attr4,:Y,:Attr)])
@acset_type DataAttrs2(SchAttrs2)

mydata = @acset DataAttrs2{Int} begin
   X=3
   Y=2
end

attr1=[1,2,3]
attr2=[4,5,6]

attr3=[7,8]
attr4=[9,10]

set_subparts!(
   mydata,
   attr1=attr1, attr2=attr2, attr3=attr3, attr4=attr4
)

@jpfairbanks
Copy link
Member

I think that this api makes sense to me. Although is there a readability problem with omitting the : argument? I'm just thinking about the design principle that explicit is better than implicit.

You would check the arguments for length and then dispatch to putting the colon in. I guess that makes sense to me. Any objection from @epatters or @olynch?

@slwu89
Copy link
Member Author

slwu89 commented Sep 16, 2023

There's no problem, I was just curious about the apparent inconsistency of allowing a user to omit the parts argument in set_subpart! but not in set_subparts!.

@slwu89
Copy link
Member Author

slwu89 commented Feb 13, 2024

This came up again with a new user, so want to ask @epatters, should set_subparts! be allowed to set subparts of different objects? Or should it only be able to set subparts of one object?

@jpfairbanks
Copy link
Member

Well, I guess it is fine to allow setting subparts for multiple tables in the same operation when the row index is omitted. So in you example:

using ACSets

SchAttrs2 = BasicSchema([:X,:Y], [], [:Attr], [(:attr1,:X,:Attr),(:attr2,:X,:Attr),(:attr3,:Y,:Attr),(:attr4,:Y,:Attr)])
@acset_type DataAttrs2(SchAttrs2)

mydata = @acset DataAttrs2{Int} begin
   X=3
   Y=2
end

attr1=[1,2,3]
attr2=[4,5,6]

attr3=[7,8]
attr4=[9,10]

# this should work because you are referring to all the rows in the table
# and the order of the rows needs to match the order of the columns you are setting.
set_subparts!(
   mydata,
   attr1=attr1, attr2=attr2, attr3=attr3, attr4=attr4
)

# this should NOT work because the entries of the list [1,3] are referring to specific rows of the X 
# table (for `attr{1,2}`) or specific rows of the Y table (for `attr{3,4}`). 
# And we will only introduce errors if people luck into getting the 
# correct results sometimes when the rows happen to be the same.
set_subparts!(
   mydata, [1,3]
   attr1=attr1[1:2], attr2=attr2[1:2], attr3=attr3[1:2], attr4=attr4[1:2]
)

@slwu89
Copy link
Member Author

slwu89 commented Feb 13, 2024

sounds good, thanks @jpfairbanks!

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

No branches or pull requests

2 participants