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

At/docs #90

Merged
merged 20 commits into from
Jun 28, 2021
Merged

At/docs #90

merged 20 commits into from
Jun 28, 2021

Conversation

thazhemadam
Copy link
Member

Add a few docstrings, and new section for Codecs

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2021

Codecov Report

Merging #90 (98fc669) into main (c5bde65) will not change coverage.
The diff coverage is n/a.

❗ Current head 98fc669 differs from pull request most recent head eda6757. Consider uploading reports for the commit eda6757 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main      #90   +/-   ##
=======================================
  Coverage   81.08%   81.08%           
=======================================
  Files          11       11           
  Lines         312      312           
=======================================
  Hits          253      253           
  Misses         59       59           
Impacted Files Coverage Δ
src/features/elementfeature.jl 82.35% <ø> (ø)

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 c5bde65...eda6757. Read the comment docs.

@rkurchin
Copy link
Member

Can you grab commits 3c7032 through 4581d1c from #87 (excluding 04a1b78, I don't think we need to tag this) and add them to this since that's probably not going to get merged and those definitely fit under this header?

Also, for my own educational purposes, can you send me the lines of git that you run to do that? 🥺

@rkurchin
Copy link
Member

(All the other stuff here looks great to me, thanks for adding those missing docstrings!)

output_shape(efd::ElementFeatureDescriptor) = output_shape(efd, efd.encoder_decoder)

"""
output_shape(efd::ElementFeatureDescriptor, ed::OneHotOneCold)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this actually needs to be a separate docstring?

Copy link
Member Author

Choose a reason for hiding this comment

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

fair enough.

test/runtests.jl Outdated
@@ -4,6 +4,7 @@ using Test
const testdir = dirname(@__FILE__)

tests = [
"module_tests.jl"
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs a comma and then the CI should pass again ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, fixing it now.

@rkurchin rkurchin mentioned this pull request Jun 28, 2021
rkurchin and others added 16 commits June 29, 2021 00:09
Fixed this in main module file

Signed-off-by: Rachel Kurchin <[email protected]>
this must not be covered by tests because featurizations aren't lists anymore so we can't iterate on them

Signed-off-by: Rachel Kurchin <[email protected]>
Signed-off-by: Rachel Kurchin <[email protected]>
Signed-off-by: Rachel Kurchin <[email protected]>
It used to be a Union type included Vector{Integer}, but it should have been Vector{<:Integer}

Also added a test to disambiguate this case a bit better.

Signed-off-by: Rachel Kurchin <[email protected]>
formatting

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
`ElementFeatureDescriptor` has been updated to include `encoder_decoder`
field, and exclude the `length` and `logspaced` fields. Update the
docstring of the type to reflect this.

Accordingly, create a doctring for `ElementFeatureDescriptor`'s
constructor as well.

Signed-off-by: Anant Thazhemadam <[email protected]>
Create docstrings for the `default_efd_encode` and `default_efd_decode`
(the default encode and decode functions for an ElementFeatureDescriptor
respectively).

Signed-off-by: Anant Thazhemadam <[email protected]>
Signed-off-by: Rachel Kurchin <[email protected]>
Test the functions `encodable_elements`, `decode`, and `featurize!` that
are defined at the module level

Signed-off-by: Rachel Kurchin <[email protected]>
Signed-off-by: Anant Thazhemadam <[email protected]>
module_tests had .jl and it was getting added again, removed it so CI should hopefully pass now
test/module_tests.jl Outdated Show resolved Hide resolved
test/module_tests.jl Outdated Show resolved Hide resolved
rkurchin and others added 2 commits June 28, 2021 16:12
following formatter suggestions

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
formatter suggestion

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
test/module_tests.jl Outdated Show resolved Hide resolved
@rkurchin rkurchin merged commit 6123ed5 into main Jun 28, 2021
@rkurchin rkurchin deleted the at/docs branch June 28, 2021 20:28
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