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

Update scitypes.jl #29

Merged
merged 1 commit into from
Dec 23, 2021
Merged

Update scitypes.jl #29

merged 1 commit into from
Dec 23, 2021

Conversation

OkonSamuel
Copy link
Contributor

@juliohm
Copy link
Member

juliohm commented Dec 23, 2021

So we should only depend on ScientificTypesBase.jl now @OkonSamuel ?

@OkonSamuel
Copy link
Contributor Author

OkonSamuel commented Dec 23, 2021

So we should only depend on ScientificTypesBase.jl now @OkonSamuel ?

Yes. Only extend methods from ScientificTypesBase. But you would still need to import ScientificTypes to use scitype(c)

julia> using CoDa, ScientificTypes

julia> c = Composition(1,2,3)
                  3-part composition
      ┌                                        ┐
   w1 ┤■■■■■■■■■■■■ 1.0
   w2 ┤■■■■■■■■■■■■■■■■■■■■■■■ 2.0
   w3 ┤■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■ 3.0
      └                                        ┘

julia> scitype(c)
Compositional{3}

@juliohm
Copy link
Member

juliohm commented Dec 23, 2021

But it seems that we still need DefaultConvention from ScientificTypes.jl? It is a bit confusing. Can you please clarify?

@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2021

Codecov Report

Merging #29 (7b5d14d) into master (cae693f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #29   +/-   ##
=======================================
  Coverage   84.50%   84.50%           
=======================================
  Files          13       13           
  Lines         355      355           
=======================================
  Hits          300      300           
  Misses         55       55           
Impacted Files Coverage Δ
src/scitypes.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cae693f...7b5d14d. Read the comment docs.

@OkonSamuel
Copy link
Contributor Author

OkonSamuel commented Dec 23, 2021

But it seems that we still need DefaultConvention from ScientificTypes.jl? It is a bit confusing. Can you please clarify?

Yes, You would still need ScientificTypes, DefaultConvention.

So that could be done by

ST.ScientificTypesBase.scitype(::YourType, ::ST.DefaultConvention) = ...

I hope this is clear enough.

@juliohm
Copy link
Member

juliohm commented Dec 23, 2021

I find it strange that ScientificTypes.jl doesn't reexport scitype from ScientificTypesBase.jl. Someone depending on the default convention would like to implement methods for functions defined in the same module. Any chance these functions can be reexported in ScientificTypes.jl? I really feel that we should try another round of refactoring in these two packages.

@juliohm juliohm merged commit f6d1dd5 into JuliaEarth:master Dec 23, 2021
@OkonSamuel
Copy link
Contributor Author

I find it strange that ScientificTypes.jl doesn't reexport scitype from ScientificTypesBase.jl. Someone depending on the default convention would like to implement methods for functions defined in the same module. Any chance these functions can be reexported in ScientificTypes.jl? I really feel that we should try another round of refactoring in these two packages.

I'll discuss with @ablaom about this possibility.

@OkonSamuel
Copy link
Contributor Author

@juliohm what if I renamed the scitype method from ScientificTypesBase as scitype_def and exported that?
So you could do

using CoDa, ScientificTypes

ScientificTypes.scitype_def(::YourType, ::ST.DefaultConvention) = ...

c = Composition(1,2,3)

scitype(c)

@juliohm
Copy link
Member

juliohm commented Dec 23, 2021

Why do we need two different function names @OkonSamuel ? Isn't just a single function with multiple methods we are discussing? Something changed recently? Why things are split between two packages the way they are?

@OkonSamuel
Copy link
Contributor Author

OkonSamuel commented Dec 23, 2021

Why do we need two different function names @OkonSamuel ? Isn't just a single function with multiple methods we are discussing? Something changed recently? Why things are split between two packages the way they are?

Yes, there was a recent change as regards performance, we wanted to avoid setting convention at run time.

We can't just move the DefaultConvention implemted in ScientificTypes to ScientificTypesBase.jl because we wish that ScientificTypesBase.jl be as light as possible.

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