search

afids / afids-validator

Validator for the anatomical fiducial placement protocol
https://validator.afids.io
GNU General Public License v3.0
2 stars 4 forks source link

Fixes #7 #9

Closed kaitj closed 5 years ago

kaitj commented 5 years ago

@tkkuehn - Should be able to use additions for #8

kaitj commented 5 years ago

I believe the original version of the validator was comparing two fcsv/csv files? I agree with comments. Can't recall why we wanted to go fcsc to json. Looking back, seems redundant to go from fcsv to json for the comparison. Are there any benefits to converting to json first?

@Park-Patrick @ostanley @jclauneuro thoughts?

To the last point of python dict and json object, Python's json library does a pretty good job of going between the two.

jclauneuro commented 5 years ago

+1. I don't see any advantage of creating our own internal representation in JSON, particularly given that figuring out how to represent discrete coordinates seems likely to be a priority in 2019 within the BIDS community: https://groups.google.com/forum/#!topic/bids-discussion/rQPf6DuVbh0

Nice to know that converting between object types seems relatively straightforward though. Anything else I've forgotten to comment on in this thread?

kaitj commented 5 years ago

Just an additional thought and maybe it was why we wanted to convert it to JSON in the first place.

We wouldn't constrained by the limitations of a csv reader (eg. row-by-row computations) if it it is in a python dict/JSON object. I'm not aware of any way of probing the CSV directly and the limitation would be row-by-row computations. The caveat is we would have to ensure the inputted file is structured how we want it (see #8 - afids in ascending order would have to be checked before hand, or we could add that column to the JSON object for the comparison).

jclauneuro commented 5 years ago

Thanks for the clarification, @kaitj! That could be very well the reason why!

I have no experience with trying to parse through JSON files (for analysis, I live in an R dataframe world) but that definitely sounds like an advantage. We're ultimately only talking about 5 columns/values per "row" so the implementation cost of creating our own JSON format is likely minimal?

ostanley commented 5 years ago

You guys seem to have a handle on this but I agree with @kaitj that json will be more flexible long term. We could always switch back later.

tkkuehn commented 5 years ago

Okay, I'm on board with the JSON structure we've settled on and the current version of the PR looks good to me.

If we're going to merge this and #11, I think it makes sense to merge #11 first, bring those structural changes in here, then merge this. @kaitj if you agree I can handle that.

ostanley commented 5 years ago

Great then I'll update the whole thing on our python anywhere and add the links to the git repo so we are not manually uploading. Easier once these two are merged in.

kaitj commented 5 years ago

@tkkuehn Sounds good to me! I think this fork/branch is as updated as needs to be at this point. I've made the changes to the variable/function names and updated the JSON structure as discussed.