scikit-tda / tadasets

Synthetic data sets apt for Topological Data Analysis
http://tadasets.scikit-tda.org
MIT License
34 stars 7 forks source link

Initial work on adding possible rotation of infinity sign. #8

Closed Filco306 closed 3 years ago

Filco306 commented 3 years ago

Hello!

Thank you for a great repository! I wanted and needed to rotate the infinity sign, so I though I might add it here.

Basically, I add an argument to allow for rotating an infinity sign. As an example, using angle=np.pi/2 will generate an eight.

Best, Filip

codecov[bot] commented 3 years ago

Codecov Report

Merging #8 (cd9c19d) into master (bbc3162) will increase coverage by 3.76%. The diff coverage is 79.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
+ Coverage   86.92%   90.69%   +3.76%     
==========================================
  Files           6        7       +1     
  Lines         153      172      +19     
  Branches       13       14       +1     
==========================================
+ Hits          133      156      +23     
+ Misses         15       10       -5     
- Partials        5        6       +1     
Impacted Files Coverage Δ
tadasets/sample.py 100.00% <ø> (ø)
tadasets/view.py 100.00% <ø> (ø)
tadasets/shapes.py 80.00% <69.23%> (+10.30%) :arrow_up:
tadasets/rotate.py 86.66% <86.66%> (ø)
tadasets/_version.py 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 bbc3162...ce5f693. Read the comment docs.

Filco306 commented 3 years ago

I think the conflicts arose after the recent pull request was merged in.

Filco306 commented 3 years ago

Fixed the merge conflicts; both the seed PR and my PR are integrated now.

sauln commented 3 years ago

Looks good! just a small tweak to leverage pytest.raises in the tests and we're good to go 👍

Filco306 commented 3 years ago

This is actually great, I didn't know about pytest.raises. Great feedback on the code, thank you for that! :D

Filco306 commented 3 years ago

It looks like both PRs are merged into 1 now? I'm okay with 1 big one if that was your intention.

A couple of small tasks before we merge it:

  • Could you run black over the files you changed to get the formatting in line?
  • Could you test the docs to make sure they show the new changes you've made to the api?
  • Could you bump the version in tadasets/_version.py? Thank you!

Hello! I accidentally merged the other PR into this, but I reverted it, so this only contains the changes with the infty sign now :)

make html
Running Sphinx v3.3.1

Configuration error:
There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/sphinx/config.py", line 319, in eval_config_file
    execfile_(filename, namespace)
  File "/usr/local/lib/python3.8/site-packages/sphinx/util/pycompat.py", line 89, in execfile_
    exec(code, _globals)
  File "/Users/filipcornell/Desktop/code/forks/tadasets/docs/conf.py", line 7, in <module>
    from sktda_docs_config import *
ModuleNotFoundError: No module named 'sktda_docs_config'

make: *** [html] Error 2

Again, I am new to sphinx so it might be a noob error.

Again, your feedback is great and thank you for your guidance! Keep pushing for these small details, they are great for me to learn new things!

(And sorry for being a bit noobish on some of these parts lol)

sauln commented 3 years ago

ModuleNotFoundError: No module named 'sktda_docs_config'

Try installing that library and then it should be good to go.

If you want to, I can add those configurations to this rep

That would be great 👍

By bumping, do you mean changing it to 0.0.5?

Let's do 0.1.0!

Filco306 commented 3 years ago

I bumped it now, but the changes does not appear in the documentation. angle and seed do not appear. Hmm

sauln commented 3 years ago

hmm indeed.. When you ran make html, did it generate a _build dir with index.html in it?

Filco306 commented 3 years ago

hmm indeed.. When you ran make html, did it generate a _build dir with index.html in it?

It did. But it does not generate it with the new parameters :/

For infty_sign, I do not seem to get any html. Strange.

sauln commented 3 years ago

Okay, no worries. Everything else looks good to go. I'll try taking a look between merging this and getting the second holes in swiss PR merged.

Is this all ready to go for you?

Filco306 commented 3 years ago

Hello!

Please excuse my late reply. I added my pre-commit linting now that uses .flake8 and black. I also fixed some minor issues that I found in different places, such as variables that were assigned but never used (those assignments were commented out) and assured that it would work. It passes all the tests and is now better code :)

On the infty-rotation, it is all done and ready to be merged :)

Thank you for all your trouble with this! I learnt a lot by contributing here!

Cheers, Filip

sauln commented 3 years ago

As with the other one, what are your thoughts on merging it? It's been a bit so I can't remember where it was left. Ready to go?

Filco306 commented 3 years ago

Hello @sauln , please excuse my very late reply! This one is ready to go. I think the other one, although it is interesting, it does need some more work because it cannot scale with as the number of dimensions scale.

Again, sorry for such a late reply, but this one is ready to go :)

sauln commented 3 years ago

Sweet! Here it goes!