ENH: Initial commit of composite database

ostanley commented 3 years ago

Proposed changes

This is an initial commit of the new database. Several open questions remain, not limited to:

Dynamic naming of the aifds without using exec.
Where to put the database in the project (can use from app import db to move it).
Testing fully.

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

[ ] Bugfix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionalitiy)
[x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Other (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!

[x] Changes have been tested to ensure that fix is effective or that a feature works.
[x] Changes pass the unit tests
[x] Code has been run through black with the -l 79 flag.
[ ] I have included necessary documentation or comments (as necessary)
[x] Any dependent changes have been merged and published

Notes

All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.

_PR template was adopted from appium_

ostanley commented 3 years ago

All tests passing on the new database when not using composite points.

Execs remain everywhere because insertion by dictionary requires some more reading on my end. I wanted to figure that out before I did the composite points in case it changes things.

ostanley commented 3 years ago

Status update:

[x] Merged with master
[x] Database read and write tested and passing
[x] Database table appears as expected
[x] Multiple tables allowed
[ ] Dynamic table building created
[x] Confirmed composites are actually saving time NOT adding complexity

ostanley commented 3 years ago

Also need to add name check back to the validator. Each fiducial must be on the expected list.

ostanley commented 3 years ago

The name check is intrinsic to0 the for loop. It checks from the map of names so it checks each point it expects. If there were extra points the number of validators would be >32 or the for loop would fail.

ostanley commented 3 years ago

Yes, visualization.py and controller.py need integration with this branch prior to merging it in. Also, we need to finalize the different tables for afids. Re: migration I thought we were dropping the old database as it is all test data so migration won't be needed?

ostanley commented 3 years ago

Update: website is now using the new db. Everything is running for both .fcsv and .json test files. Saving works and dumping the saved data works.

I had to move around some of the imports to avoid a circle since we moved the db class to model. If someone could check that especially well when reviewing I'd appreciate it.

Also, two things that will need future adaption:

csv_to_afids and json_to_afids both initialize a HumanFiducialSet at the moment. This will have to get smarter when macaque stuff is merged in.
User_id column is currently not used at all and not saved to database.

ostanley commented 3 years ago

Migration added, it does not say create tables only to drop them. I think this is fine as it should create them as required but I am just getting comfortable with migration so let me know if not.

kaitj commented 3 years ago

Fixed the postgressql service for CI in 9fef45c, should be working now.

tkkuehn commented 3 years ago

Migration added, it does not say create tables only to drop them. I think this is fine as it should create them as required but I am just getting comfortable with migration so let me know if not.

Having taken a look at it, the migration does not appear to have run properly -- It should also create the human_fiducial_set table that is now defined in models.py. It doesn't look like Flask-Migrate is actually properly set up now, and a look at the project history suggests that it's never been set up (i.e. initialized after app with something like migrate = Migrate(app, db)). I did a quick test of this on my end, and I would suggest deleting the migration that's checked in right now, making sure Flask-Migrate gets initialized, and trying the migration again.

kaitj commented 3 years ago

Testing (at least in CI) is currently printing the contents of the test inputs. Can see a log here: https://github.com/afids/afids-validator/runs/3117352410?check_suite_focus=true

Not sure if it is CI-only or also local.

afids / afids-validator