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

Add gif to markdown to display usage #141

Merged
merged 1 commit into from
Dec 17, 2021
Merged

Add gif to markdown to display usage #141

merged 1 commit into from
Dec 17, 2021

Conversation

melhemr
Copy link
Contributor

@melhemr melhemr commented Dec 16, 2021

No description provided.

Copy link
Contributor

@AbdulkadirA AbdulkadirA left a comment

Choose a reason for hiding this comment

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

@melhemr Thanks. The animation works and looks nice. It is a good demo. The length, however, could be substantially reduced by cutting the video to show only relevant actions. Here are some suggestions:

  • Age trends is shown with too many categories
  • The text harmonization is too crowded. Consider reducing the number of sites that are included to simplify the view.
  • File selection dialogs should show de-cluttered folders. Make a folder with only data, harmonization model, and SPARE-* model. Set the working directory such that it starts in the desired folders and no browsing is necessary.
  • Choose a data set with only a small number of categorical columns; e.g. Sex, Diagnosis, APOE, Study, and SITE.
  • If possible, use the newest commit.
  • GIF should be included in the repository (preferably as LFS file)

@melhemr
Copy link
Contributor Author

melhemr commented Dec 17, 2021

@melhemr Thanks. The animation works and looks nice. It is a good demo. The length, however, could be substantially reduced by cutting the video to show only relevant actions. Here are some suggestions:

  • Age trends is shown with too many categories
  • The text harmonization is too crowded. Consider reducing the number of sites that are included to simplify the view.
  • File selection dialogs should show de-cluttered folders. Make a folder with only data, harmonization model, and SPARE-* model. Set the working directory such that it starts in the desired folders and no browsing is necessary.
  • Choose a data set with only a small number of categorical columns; e.g. Sex, Diagnosis, APOE, Study, and SITE.
  • If possible, use the newest commit.
  • GIF should be included in the repository (preferably as LFS file)
  1. is there a way to set the default category to sex so that I don't have to change the category to show a good image?
  2. How do I set the working directory for file searches?

@AbdulkadirA
Copy link
Contributor

  • is there a way to set the default category to sex so that I don't have to change the category to show a good image?

Since you want to have only a limited number of columns showing up in the drop down and fewer sites anyway, you should use a data set that is void of problematic categories and has a smaller number sites.

@AbdulkadirA
Copy link
Contributor

2. How do I set the working directory for file searches?

I think it is the argument dir in getOpenFileName

https://doc.qt.io/qtforpython-5/PySide2/QtWidgets/QFileDialog.html#PySide2.QtWidgets.PySide2.QtWidgets.QFileDialog.getOpenFileName

@melhemr
Copy link
Contributor Author

melhemr commented Dec 17, 2021

@AbdulkadirA One last question, what do you mean use newest commit? The branch is up-to-date

@AbdulkadirA
Copy link
Contributor

@AbdulkadirA One last question, what do you mean use newest commit? The branch is up-to-date

If the branch you are using to create the animation is up-to-date with main, that's fine. There was a light blue button with white font. I thought that had been updated to black font already.

Copy link
Contributor

@AbdulkadirA AbdulkadirA left a comment

Choose a reason for hiding this comment

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

@melhemr Thanks. There was a category 1 in the SPARE-* plot. Could you remove that? It looks overall good, a bit long but acceptable for the initial demo.

Copy link
Contributor

@AbdulkadirA AbdulkadirA left a comment

Choose a reason for hiding this comment

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

ok

@AbdulkadirA AbdulkadirA merged commit e88b59a into CBICA:main Dec 17, 2021
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.

2 participants