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

First version of the ioSPI notebook tutorial #52

Closed
wants to merge 4 commits into from

Conversation

luis-sribeiro
Copy link
Collaborator

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ninamiolane
Copy link
Contributor

@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

@@ -0,0 +1,431 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

That's great! It would be even better if we had a "real-life" example


Reply via ReviewNB

@@ -0,0 +1,431 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Are we expecting something to be printed here?


Reply via ReviewNB

@@ -0,0 +1,431 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Nice!


Reply via ReviewNB

@fredericpoitevin
Copy link
Member

@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
Copy link
Contributor

geoffwoollard commented Mar 18, 2022

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 First version of the ioSPI notebook tutorial #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
Copy link
Contributor

@luis-sribeiro I confirm that the above steps all worked for me, including this PR that passed: #61

@fredericpoitevin
Copy link
Member

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)

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.

4 participants