-
Notifications
You must be signed in to change notification settings - Fork 13
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
Release/v3.0.0 #50
Release/v3.0.0 #50
Conversation
This commit makes the `IntervalCollection` element type generic, which is a breaking change.
Benchmark resultJudge resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to pretend I understand the scope of these changes. I can do a more detailed review if you think that would be helpful, but I trust you to make good decisions here.
My only high-level comment is that all doctests should probably pass (or be removed) at this stage before merging. No reason to have our first post-1.0 docs be instantly outdated 😉
a9736a7
to
6e97f12
Compare
@kescobo, this PR is now ready for review again. I think I addressed your comment and tidied up a few loose ends. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a bunch of comments, but the vast majority amount to style preferences, and I do not feel strongly enough to consider them blocking. There is one spot where it looks like a T
didn't get correctly changed to I
(or vice versa), but that's the only part of this review that I would insist on.
The big style ones that show up in a few places look as if you were copying the previous style, but I think should be changed are
- A bunch of block that have the struture
if ... return X; end; if ... return Y; end; return Z
that I think should either beif/elseif/else
, or else use short-circuiting&& return X
- A bunch of function definitions that are are just returning something that I think could be written with
foo() = X
rather thanfunction foo(); return X; end
In both cases, I commented on one or two examples, but there were many more that I didn't flag. If you do want to make these changes but don't want to put in the extra tedious effort, I can do them as a separate PR - neither of them should change the functionality, so they can safely be done after v3.0 is tagged.
Develops `intervaltype` methods.
- Rename `Interval` to `GenomicInterval`. - Rename `IntervalCollection` to `GenomicIntervalCollection`.
Allow users to develop their own custom concrete types.
6e97f12
to
e24882b
Compare
e24882b
to
097a253
Compare
2ab63cc
to
91712bf
Compare
Benchmark resultJudge resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Co-authored-by: Kevin Bonham <[email protected]>
Benchmark resultJudge resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/GenomicFeatures.jl/GenomicFeatures.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Thanks @kescobo. |
This PR refactors
GenomicFeatures
to make use of abstract types.This PR contains the following breaking changes.
Interval
type is renamedGenomicInterval
and now subtypesAbstractGenomicInterval
.IntervalCollection
is nowGenomicIntervalCollection
, and the element type of the collection is now theAbstractGenomicInterval
instead of the interval's metadata type.groupname
instead ofseqname
.