radiocosmology / padloper

MIT License
2 stars 0 forks source link

Refactor child classes #108

Closed ahincks closed 7 months ago

ahincks commented 8 months ago

A huge commit that refactors things to avoid repeating lots of similar code in the child classes of Vertex. The API to the external user should be basically the same as before, with the main exception being that you need to explicitly set the attributes when initialising a Vertex child instance: e.g., p.ComponentType(name="mytype", comments="sldkfj") rather than p.ComponentType("mytype", "sldkfj").

I think this PR should be merged now, but later I plan on some more modifications:

ahincks commented 8 months ago

All of those attributes are included in the parent, Vertex, class, so all the children inherit them and they shouldn't be included as _VertexAttr instances.

But you're right that as_dict() should include these. Let me make the fix.

ahincks commented 8 months ago

OK, made the fix. I've tested it quickly on the web interface and seems to work.

jfitzgerald1126 commented 8 months ago

I see, that makes more sense. However, I also noticed that when I checked out the branch and cleared the database, any new components added didn't represent lists with multiple values properly. For example, Property vertices would only store the last value in the values list if multiple values were defined. I'm not sure what caused this issue because the issue resolved when I restored the old database I had.

ahincks commented 8 months ago

I'm not sure I follow completely. Is it simply that when you are creating a Property not all the values are being stored? I just checked and I am able to store multiple values. Maybe you could create a reproducible example of the problem you're having?

jfitzgerald1126 commented 8 months ago

Yes, I cleared the database because the components I had caused key errors on either init or from_db because they were missing the uid_added property. With the fresh database, any new properties I added only stored the last property in a list, for example [1, 2, 3] would just be stored as [3], and cause errors. I even created a fresh instance from main and a new JanusGraph install, but had the same issue. I was only able to resolve the property issue by restoring the database I had previously.

I'm not sure what caused this issue but I can see if I can reproduce it.

ahincks commented 8 months ago

OK. Did you try running padloper/scripts/tests.py? One thing it does is create properties and add them to components, and it works for me with multiple values in the components. But if you have an example where things break obviously I want to know!

jfitzgerald1126 commented 8 months ago

Resetting the database fixed the previous issue, but I'm still not sure what caused it. Everything seems to be working except n_values is being returned as a string from queries to the DB in from_db, get_list, etc. This is causing type errors on __init__ calls.

ahincks commented 8 months ago

I sent this to @jfitzgerald1126 in Slack but then realised I should probably have put it here, so let me cut and paste and lightly edit: