search

compSPI / ioSPI

I/O and Data Visualization
MIT License
4 stars 7 forks source link

First version of the ioSPI notebook tutorial #52

Closed luis-sribeiro closed 2 years ago

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

ninamiolane commented 2 years ago

@luis-sribeiro thanks for this! Any idea why the tests are not passing?

We also need a file that will test this notebook, e.g. like this file: https://github.com/geomstats/geomstats/blob/master/tests/test_notebooks.py

fredericpoitevin commented 2 years ago

@luis-sribeiro thanks for this! Any idea why the tests are not passing?

@luis-sribeiro it looks like a credential issue. I have not looked into it carefully: is there some documentation somewhere about the access token and how to manage them?

We also need a file that will test this notebook, e.g. like this file: https://github.com/geomstats/geomstats/blob/master/tests/test_notebooks.py

Yes, that would be awesome!

geoffwoollard commented 2 years ago

Assuming I understood things from @fredericpoitevin and @arjunsingh3600 :

@luis-sribeiro , I suggest you

  1. Make a repo of the main compSPI:ioSPI fork locally
  2. Make an empty feature branch on the main compSPI fork, off master. It will be just an empty feature branch. Let's call this compSPI:luis_notebook
  3. Do a PR of your new feature branch from your fork (luis-sribeiro:master for PR https://github.com/compSPI/ioSPI/pull/52) to compSPI:luis_notebook
  4. Now the feature is within the compSPI organization, and the authentication will work differently and the PR won't fail.
  5. Now do a PR of compSPI:luis_notebook --> compSPI:master

In the future you can avoid this by working on the compSPI organization repo, instead of your own fork. This is what @arjunsingh3600 has been doing and that's why his PRs have different behaviour.

It should just take a few moments and would be easy to check if it works.

geoffwoollard commented 2 years ago

@luis-sribeiro I confirm that the above steps all worked for me, including this PR that passed: https://github.com/compSPI/ioSPI/pull/61

fredericpoitevin commented 2 years ago

I created the luis_master branch on the main fork and tests pass there: #64

@luis-sribeiro I am going to close this pull request - could you please address the comments in the other one (you can push to origin luis_master to update it)