isi-vista / adam

Abduction to Demonstrate an Articulate Machine
MIT License
11 stars 3 forks source link

GNN module #1151

Closed spigo900 closed 2 years ago

spigo900 commented 2 years ago

This seems to be working. I'm planning to clean up the history and probably the code more the next working day.

I've tried to keep the code mostly as Sheng wrote it although I've renamed some variables and of course the data loading logic is entirely different.

For some reason PyCharm thinkstorch and numpy are both unused in these scripts... :shrug: ETA: Figured it out. It's because utils imports those those modules and the scripts do a * import from utils.

spigo900 commented 2 years ago

This should be ready for review now. I've cleaned up the history. I've also blackened the sections of the two scripts that I changed. I left the rest as-is.

I haven't set up the Makefile to run checks on this directory given the different style of the original code. I did manually run some checks. flake8 and pylint are mostly happy except for some quirks preserved from the original scripts. Mypy is grumpy about ndarray not having type parameters -- it looks like the second should be a dtype but I haven't yet found an explanation for the first parameter.

spigo900 commented 2 years ago

I've tested and it seems the stroke extraction code is working. Results are inconsistent with Sheng's, however I've reproduced that inconsistency using the original code so this inconsistency is not due to any of the changes I've made. So I think this code is ready for review.

I am separately checking to see if I get consistent results when using the same Matlab version Sheng used. That shouldn't affect this pull request however.

spigo900 commented 2 years ago

Travis seems to have lost track of this PR, so if no one objects by 11, I'm going to force-merge it. It passed pre-commit last time Travis ran and the only changes were to the README and to add some comments so it should still.