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 abstract type AbstractCircularArray #30

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jack-dunham
Copy link

I think it would be useful to have an abstract type here that assumes circular indexing.

With this implementation, types that subtype AbstractCircularArray will work out of the box provided the parent array is stored in a field data. If this is not the case then Base.parent should be overloaded.

All tests pass so this should hopefully not break anything.

If you are interesting in merging, I will add docs and some tests.

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ec93323) 100.00% compared to head (b13a4c3) 98.03%.

❗ Current head b13a4c3 differs from pull request most recent head 89f76a9. Consider uploading reports for the commit 89f76a9 to get more accurate results

Files Patch % Lines
src/CircularArrays.jl 97.36% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##            master      #30      +/-   ##
===========================================
- Coverage   100.00%   98.03%   -1.97%     
===========================================
  Files            1        1              
  Lines           50       51       +1     
===========================================
  Hits            50       50              
- Misses           0        1       +1     
Flag Coverage Δ
unittests 98.03% <97.36%> (-1.97%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@putianyi889
Copy link
Contributor

can you keep the owner's code styles, e.g. spaces and braces?

@jack-dunham
Copy link
Author

I have removed the accidental formatting of the files, and fixed the commit history.

Does this repo adhere to any particular style? I wasn't able to reconcile the owners code style with options in JuliaFormatter.

Cheers.

@putianyi889
Copy link
Contributor

I have removed the accidental formatting of the files, and fixed the commit history.

Does this repo adhere to any particular style? I wasn't able to reconcile the owners code style with options in JuliaFormatter.

Cheers.

changing the style brings a lot of fake changes making the history harder to track.

@jack-dunham
Copy link
Author

Yeah I understand that, but it would be useful if the author could commit a .JuliaFormatter.toml corresponding to their preferred formatting.

I'll create an issue.

@Vexatos
Copy link
Owner

Vexatos commented Apr 18, 2024

Style issues aside, I am not sure I like this idea. CircularArray is already generic in the sense that it can wrap any other type of array. In what kind of situation would adding the abstract type add new functionality? Any custom type extending AbstractCircularArray that can't already use the generic CircularArray type ought to be so specialized that the creator of such a type should probably just make its own type entirely from scratch.

@jack-dunham
Copy link
Author

This feature is motived by the following problem:

In package A I have defined two methods for func, one for CircularArray and one for AbstractArray.

In package B I have MyType <: AbstractArray that wraps CircularArray. I am unable to use MyType in package A as the not circular-indexing method of func will be called.

If I could define the func method on AbstractCircularArray then MyType could subtype this and everything would work automatically. Currently, my solution to this is to define my own abstract type in package A that assumes circular indexing, and then subtype this. But then this means package A must be a hard dependency of B when it is otherwise unnecessary.

I understand I could just unwrap and rewrap with CircularArray whenever needed but this is added tedium in both coding and interface that could be avoided by the existence of an abstract type.

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.

3 participants