isi-usc-edu / qb-gsee-benchmark

Apache License 2.0
0 stars 0 forks source link

Fixes to schemas and problem instances files #22

Closed max-radin closed 1 week ago

max-radin commented 1 week ago

This PR fixes a few mistakes in the solution schema and also updates the problem instances that were removed by CI in the last commit to comply with the new schemas. Note that each task has been assigned a new UUID that differs from the fcidump UUIDs, as discussed in https://github.com/isi-usc-edu/qb-gsee-benchmark/pull/14.

jtcantin commented 1 week ago

Note: Combining these changes with those I made still in progress

jp7745 commented 1 week ago

Just want to make sure we're not stepping on each other here...

(A) I believe Josh is updating problem_instance files and this PR also updates problem_instance files.

(B) Josh also has an open PR for fixes to the solution.schema in PR#19. When I check git diff max-radin/schema-fixes jtcantin-patch-1 -- schemas/solution.schema.0.0.1.json, I do see there are some differences between the solution.schema fixes... most appear to be formatting.

Talk to each other to ensure that (A) and (B) are good. Let me know the resolution.

jtcantin commented 1 week ago

I've merged Max's schema changes into mine Problem instances still pending

jtcantin commented 1 week ago

@max-radin It looks like most of the updates are just adding task ids, while some problem instances have additional reformatting. Are you updating from Nicole's latest problem instances (PR#20: https://github.com/isi-usc-edu/qb-gsee-benchmark/pull/20 ), or from a version previous to that?

max-radin commented 1 week ago

No, it doesn't include Nicole's updated versions of ru_macho and fe_red. It only updates the problem instances that got marked as invalid after #14 got merged. Regarding the changes to the problem instance files, the only materials changes are renaming instance_data to tasks and adds task_uuid fields. The formatting changes are not important. Also I see now that git diff is showing no differences in the schema between #22 and #19.

@jtcantin is it safe to say that jtcantin-problem_instance_updates is more or less ready to merge in? If so, I'd suggest we close this PR and then merge #19 and then merge that branch.

On the other hand, if it's going to take some time to get jtcantin-problem_instance_updates merged in, I'd suggest we merge this PR in first. Mainly I want to make sure we have at least some problem instances available soon so that #13 isn't blocked.

As an aside: I think whenever we make breaking changes to the schema, we should try to update the JSON files in the same PR.

jtcantin commented 1 week ago

Ok, since there are no more changes that I need to incorporate, I will go ahead with PR19 and then merge in the problem instances and the DMRG solution files.

jtcantin commented 1 week ago

Addressed in #19