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

implementation ChromBackend #23

Merged
merged 5 commits into from
Sep 2, 2024
Merged

implementation ChromBackend #23

merged 5 commits into from
Sep 2, 2024

Conversation

philouail
Copy link
Collaborator

@philouail philouail commented Aug 22, 2024

So there is still a lot lot to do but as discussed I am trying to reduce the size of the PR. I hope that does not make it too confusing.

The big changes are the clear separation between what is a peaksVariables and what is a chromvariables.
See ChromBackend-functions.R

Also implementation of respective function for each such as validPeaksData() and validChromData()

Quick question:

  • I added a replacement method for chromIndex, is that okay or in general we don't want people to change that ?
  • is it a problem that chromData() returns a data.frame and not DataFrame ? I don't really see the issue for now..

Next I will add the ChromBackendMemory() implementation, i have the code, just need to clean it.
Also will look into some maintainer stuff, if you have advices don't hesitate.

@philouail
Copy link
Collaborator Author

I changed the .yml file, the checks should not fail on warnings anymore :)

Copy link
Member

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Looks good Phili! Thanks! Just some minor change requests.

Regarding the question if chromData() should return a DataFrame or data.frame, I don't see a problem for data.frame at the moment - so, given that it is also slightly more efficient, I would maybe change to data.frame instead of DataFrame.

R/AllGenerics.R Show resolved Hide resolved
R/ChromBackend-functions.R Show resolved Hide resolved
R/ChromBackend-functions.R Show resolved Hide resolved
R/ChromBackend.R Outdated Show resolved Hide resolved
R/ChromBackend.R Outdated Show resolved Hide resolved
@philouail
Copy link
Collaborator Author

Hi so I answered / addressed the comments, thanks for the advices ! I went through the documentation a bit more realised I missed removing some stuff also.
tell me if there is something else :) and for moving the reset() to ProtGenerics also

Copy link
Member

@jorainer jorainer left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks Phili!

Before merging make sure you fix the two comments/issues in the documentation.

Regarding reset() - yes, we might want to add that to ProtGenerics, but for now define it also here. We can change later.

R/ChromBackend.R Outdated Show resolved Hide resolved
R/ChromBackend.R Outdated Show resolved Hide resolved
@philouail philouail merged commit 68a9966 into main Sep 2, 2024
6 checks passed
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