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

Impro #106

Merged
merged 3 commits into from
May 24, 2024
Merged

Impro #106

merged 3 commits into from
May 24, 2024

Conversation

iglesias
Copy link
Collaborator

Locally ctest was fine, let's see in the CI and I will take a look at examples.

@iglesias iglesias marked this pull request as ready for review May 24, 2024 11:05
@iglesias iglesias requested a review from lisitsyn May 24, 2024 11:05
Copy link
Owner

@lisitsyn lisitsyn left a comment

Choose a reason for hiding this comment

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

That's great! Shall we merge?

README.md Outdated
@@ -130,8 +125,7 @@ A minimal working example of a program that uses the library is:

MyDistanceCallback d;

TapkeeOutput output = tapkee::initialize()
.withParameters((method=MultidimensionalScaling,target_dimension=1))
TapkeeOutput output = tapke::with((method=MultidimensionalScaling,target_dimension=1))
Copy link
Owner

Choose a reason for hiding this comment

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

Oh there is a typo

Copy link
Collaborator Author

@iglesias iglesias May 24, 2024

Choose a reason for hiding this comment

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

Is it the indentation? In that case, yeah I see that maybe this line is a bit longer than the other ones with code (here in the inline diff it looks like a new line, while it is only one). Any option looks good to me, like it is with the long-ish line, or, alternatively, finishing this line at = and aligning the tapkee::with with the other with* below.

I tried but didn't manage to find a typo in the names in this line ':-D

Copy link
Owner

Choose a reason for hiding this comment

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

No it is tapke

Copy link
Collaborator Author

@iglesias iglesias May 24, 2024

Choose a reason for hiding this comment

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

Ah, finally see it, thank you!

@iglesias iglesias merged commit eb3e46c into lisitsyn:main May 24, 2024
5 checks passed
@iglesias
Copy link
Collaborator Author

I checked there were no other initialize in examples, etc.

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.

2 participants