Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object representation improvements #141

Merged
merged 36 commits into from
Oct 23, 2020
Merged

Object representation improvements #141

merged 36 commits into from
Oct 23, 2020

Conversation

marinagrprat
Copy link
Contributor

@marinagrprat marinagrprat commented Oct 6, 2020

Problem

When we call the object pyxnat it returns just a collection object and an ID number.

Feature

We want to return more information (metadata information) from the object. Some metadata information examples are the age, gender, handedness, insert data, insert user, number of subjects, experiments, scans...etc.

Solution

The changes has been done in the resources.py file.
A repr method has been added to each class (Project, Subject, Experiment, Scan and Resource).
Each method is capable to return the main information for each object dynamically (will just appear the information that the object contains, the empty fields will not appear) and a link that redirects you to the XNAT page where the object is.
This method allows us to know more information about the object immediately and find out more just clicking the link.

Unit Tests

For each object, 3 tests cases has been added. So in total, we have 15 unit tests in a new file tests/repr_test.py.
The tests check if the object exist, if the object does not exist and if it exists that the information that returns is correct.

@xgrg
Copy link
Contributor

xgrg commented Oct 8, 2020

A few comments:

  • On second thought, multiline representations may be a problem especially using lists, which is a typical use case in pyxnat (eg. lists of subjects, sessions, resources). Commits 5b93b00, 9ece9a1, bafa0e6, b8a84fe, 9a60a22 intend to provide one-line-only representations for Project, Subject, Experiment, Scan and Resource, respectively.

  • Following this, including the description of a project in the representation was potentially breaking that one-line rule:
    bc7800d provides the Project class with a desc() function to easily access a project's description instead.

  • Printing long lists was leading to some nasty overhead in execution time, due to the repetition of some generic requests (eg. interface.select('xnat:projectData').all(), etc..). Commits 5b93b00, 9ece9a1, bafa0e6 include some trick to cut this overhead by caching the results of these generic requests as private attributes of Interface.

  • For some mysterious reason (@jhuguetn is aware of this), one of these requests (xnat:subjectData) does not include subject_id in the list of attributes returned by default. 9ece9a1 therefore passes an explicit list of them to ensure subject_id is properly collected.

  • Some PEP8-related style improvements were added: lines >80 char. shortened, some variable names revised.

Thank you @marinagrprat for a nice first PR on pyxnat!

@marinagrprat
Copy link
Contributor Author

Thank you for your improvements! @xgrg

Copy link
Contributor

@jhuguetn jhuguetn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good overall, nice job here @marinagrprat and @xgrg. Find few suggestions and comments from my side in this review.

pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/core/resources.py Outdated Show resolved Hide resolved
pyxnat/tests/repr_tests.py Outdated Show resolved Hide resolved
There might be a better way to do that with some try/except
avoiding two calls to id() (one by exists()).
Also removed call to len(list(files.fetchall())) to use metadata instead
@xgrg
Copy link
Contributor

xgrg commented Oct 21, 2020

Some details on the last commits:

  • some small PEP8 style fixes
  • Added filesize to __repr__ resource (following Object representation improvements  #141 (comment))
  • Added experiment label (following Object representation improvements  #141 (comment))
  • Removed the cache workaround introduced in 5b93b00, 9ece9a1 and bafa0e6 (after it did not prove very useful)
  • Call to id() after checking for object existence (avoids issue suggested by @jhuguet)
  • subject_label in Experiment repr (was fetched incorrectly)
  • Renamed Project's desc() to description()
  • Attrs always fetch id from parent object (avoid issues w/ deleted objects)
  • Renamed nosetests3 to nosetests5 (because some smart one deleted project nosetests3 on CENTRAL... 🤦 )
  • When subject level missing, add it instead of going for the shorter URL (avoids issue mentioned by @jhuguet)

@xgrg xgrg requested a review from jhuguetn October 21, 2020 17:11
@marinagrprat
Copy link
Contributor Author

looks great! thanks @xgrg

Copy link
Contributor

@jhuguetn jhuguetn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good. I just included one minor suggestion and a couple of additional changes,

  • 468046d: Improved mechanism for collecting Scan details (~3x faster)
  • cc3b425: Outer resources from Central which are used in CI tests were mirrored into a privately owned project. Testing details updated.

pyxnat/tests/resources_test.py Outdated Show resolved Hide resolved
@xgrg xgrg merged commit 1742581 into pyxnat:master Oct 23, 2020
xgrg added a commit that referenced this pull request Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants