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 MLJ compliant docstrings #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

josephsdavid
Copy link

Need double check if they go before or after we export the model metadata!

Need double check if they go before or after we export the model metadata....
@ablaom
Copy link

ablaom commented Oct 13, 2022

If you don't use MLJModelInterface.doc_header then it doesn't matter.

@davnn
Copy link
Member

davnn commented Oct 18, 2022

Is it supposed that the MLJ docs are defined separately from the model-specific comments, e.g. there would then be https://github.com/OutlierDetectionJL/OutlierDetectionNeighbors.jl/blob/master/src/models/knn.jl#L4 and

`KNNDetector`: Identify anomalies using the distances between an instance and its k nearest neighbors.
?

@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Base: 100.00% // Head: 98.89% // Decreases project coverage by -1.10% ⚠️

Coverage data is based on head (9a8ca8b) compared to base (12565d1).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##            master       #3      +/-   ##
===========================================
- Coverage   100.00%   98.89%   -1.11%     
===========================================
  Files            7        7              
  Lines          170      181      +11     
===========================================
+ Hits           170      179       +9     
- Misses           0        2       +2     
Impacted Files Coverage Δ
src/OutlierDetectionNeighbors.jl 100.00% <ø> (ø)
src/models/dnn.jl 96.29% <0.00%> (-3.71%) ⬇️
src/utils.jl 97.05% <0.00%> (-2.95%) ⬇️
src/models/cof.jl 100.00% <0.00%> (ø)
src/models/lof.jl 100.00% <0.00%> (ø)
src/models/abod.jl 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ablaom
Copy link

ablaom commented Oct 18, 2022

I think @josephsdavid should comment here, but I think that's the current proposal - to append to the existing doc-strings. Because you also have a native API, right? However, I think we're open to a different suggestion.

@josephsdavid
Copy link
Author

Yes this was my understanding, but I am happy to adjust to however works best!

@ablaom
Copy link

ablaom commented Nov 17, 2022

@davnn Did you have any further comment on this PR? If not I will go ahead and review for compliance with the spec.

Keen to get this soon as @josephsdavid's GSoC intern finishes imminently.

@davnn
Copy link
Member

davnn commented Nov 17, 2022

I would maybe put the docs into an MLJ_docs.jl file or similar, otherwise LGTM, thanks a lot 🙏🏼

@ablaom
Copy link

ablaom commented Nov 25, 2022

Sorry for not catching this earlier but the current plan is not working out that well. The main problem is that the new docstrings are overwriting the existing ones. This may explain why @josephsdavid has duplicated the "Parameters" section, which ought not to be necessary and is bad for maintenance. It is possible to extend rather than replace a doc-string, by wrapping additions in a new module, but then the additions appear before the existing docstring, which is not what we want here.

In any case, I think splitting the doc string up is ultimately a hack and suggest abandoning that approach. I have added a POC for what I would recommend. @davnn If you would give the okay, then perhaps @josephsdavid still has time to adapt his PR along that line.

Final comment is that the MLJ examples in the current PR look untested - at least I found the ABODDetector example did not work (fixed in my POC).

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.

None yet

3 participants