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.

Sign up for GitHub

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

Jump to bottom

Query simplifications, edits in table explanations, some code refactoring #82

Merged
merged 25 commits into from
Sep 28, 2022
Merged

Query simplifications, edits in table explanations, some code refactoring #82

merged 25 commits into from
Sep 28, 2022

Conversation

tdincer
Copy link
Contributor

@tdincer tdincer commented Jun 29, 2022 •
edited by kabilar
Loading

  • Also fix query

@tdincer tdincer requested a review from dimitri-yatsenko June 29, 2022 19:42
@kabilar
Copy link
Collaborator

kabilar commented Jun 29, 2022

Thanks @tdincer. Please also update version and changelog. After this pull request is merged, please push a tag to upstream so that it can be released and published.

@kabilar kabilar self-assigned this Jun 29, 2022
@kabilar kabilar added the enhancement New feature or request label Jun 29, 2022
@kabilar kabilar added this to the 2022Q3 milestone Jun 29, 2022
@kabilar kabilar changed the title Some query optimization & small code refactoring Add imaging_withpostprocessing.py module Jun 29, 2022
@kabilar kabilar assigned CBroz1 and unassigned kabilar Jun 30, 2022
@kabilar
Copy link
Collaborator

kabilar commented Jun 30, 2022

Hi @CBroz1, would you please review and merge this pull request. @tdincer can explain the context. Thanks!

@tdincer
Copy link
Contributor Author

tdincer commented Jun 30, 2022

@kabilar I appended the query cleanings to this PR.

CBroz1
CBroz1 approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@CBroz1 CBroz1 left a comment

Choose a reason for hiding this comment

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

  1. Does the README need images or text highlighting the differences between these schemas? Or maybe the elements site?
  2. Consider copy/pasting our new code of conduct
  3. Coverage on pytests is not great - 0% on new modules, 65-75% on existing. I would prioritize updates there when we can. (grain of salt - did not run caiman or nd2 compnents)

@CBroz1 CBroz1 assigned tdincer and unassigned CBroz1 Aug 10, 2022
@tdincer tdincer closed this Aug 15, 2022
@dimitri-yatsenko
Copy link
Member

Why was this closed?

@tdincer tdincer reopened this Aug 15, 2022
@kabilar
Copy link
Collaborator

kabilar commented Aug 15, 2022

@tdincer If these changes will not be used by the Lu Lab, then we can close and reopen when we have a user requesting the use of FISSA or other post processing. Thank you.

@tdincer
Copy link
Contributor Author

tdincer commented Aug 15, 2022

@kabilar I'll try to finalize this PR today.

@tdincer @CBroz1
Update element_calcium_imaging/imaging_withpostprocessing.py
9b1d577 
Co-authored-by: Chris Brozdowski <CBrozdowski@yahoo.com>
@tdincer
Copy link
Contributor Author

tdincer commented Aug 15, 2022 •
edited
Loading

Dear all - please review the changes as soon as possible. There are some critical bug fixes in this PR.

Edit: Actually there are no critical bug fixes. Some unpushed changes on my local worried me.

@tdincer tdincer changed the title Add imaging_withpostprocessing.py module Add imaging_fissa.py module Aug 16, 2022
@kabilar
Copy link
Collaborator

kabilar commented Sep 28, 2022

Thanks @tdincer. It is not clear how FISSA is implemented with these changes. From what I understand from our conversations, the FISSA component is in the workflow for the Lu Lab. Perhaps we could implement one of the following suggestions to resolve this issue:

  1. Add the FISSA implementation within the element make function.
  2. Document how one would use these changes to implement FISSA.
  3. Rename the imaging_fissa.py module to a more generic name.

@tdincer
Copy link
Contributor Author

tdincer commented Sep 28, 2022

I removed the imaging_fissa.py. It will be issued in a separete PR.

@tdincer tdincer changed the title Add imaging_fissa.py module Query simplifications, edits in table explanations, some code refactoring Sep 28, 2022
kabilar
kabilar reviewed Sep 28, 2022
View reviewed changes
kabilar
kabilar reviewed Sep 28, 2022
View reviewed changes
tdincer and others added 2 commits September 28, 2022 09:24
@tdincer
add tag links
e3b221b
@tdincer
Update CHANGELOG.md
ca55762 
Co-authored-by: Kabilar Gunalan <kabilar@datajoint.com>
kabilar
kabilar requested changes Sep 28, 2022
View reviewed changes
Copy link
Collaborator

@kabilar kabilar left a comment

Choose a reason for hiding this comment

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

Thanks @tdincer! Great work. Minor suggestion, but the imaging modules do not all have the same changes.

@kabilar
Copy link
Collaborator

kabilar commented Sep 28, 2022

Minor suggestion, but the imaging modules do not all have the same changes.

Thanks for clarifying in person. That was my mistake. Everything looks good.

@ttngu207
Copy link
Contributor

Everything looks good to me as well, suggesting we merge @kabilar

kabilar
kabilar approved these changes Sep 28, 2022
View reviewed changes
Copy link
Collaborator

@kabilar kabilar left a comment

Choose a reason for hiding this comment

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

Thanks @tdincer! Once merged, please create a new tag and release.

@kabilar kabilar merged commit d5ad744 into datajoint:main Sep 28, 2022
kabilar pushed a commit to kabilar/element-calcium-imaging that referenced this pull request May 22, 2023
@CBroz1
Merge pull request datajoint#82 from kabilar/main
ef4331b 
Increment version to test CI/CD docker image build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimitri-yatsenko dimitri-yatsenko dimitri-yatsenko approved these changes

@CBroz1 CBroz1 CBroz1 approved these changes

@ttngu207 ttngu207 ttngu207 left review comments

@kabilar kabilar kabilar approved these changes

Assignees

@tdincer tdincer

Labels
enhancement New feature or request
Projects
None yet
Milestone
2022Q3
Development

Successfully merging this pull request may close these issues.
5 participants
@tdincer @kabilar @dimitri-yatsenko @ttngu207 @CBroz1