-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
openml integration in shogun #4628
base: develop
Are you sure you want to change the base?
Conversation
src/shogun/io/OpenmlFlow.cpp
Outdated
return 0; | ||
} | ||
|
||
const char* OpenMLReader::xml_server = "https://www.openml.org/api/v1/xml"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these things be hard-coded here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
src/shogun/io/OpenmlFlow.cpp
Outdated
|
||
if (!curl_handle) | ||
{ | ||
SG_SERROR("Failed to initialise curl handle.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe some infos on what happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what the exact error would be... From the docs: "If this function [curl_easy_init] returns NULL, something went wrong and you cannot use the other curl functions."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any value in using this other global init instead?
https://stackoverflow.com/questions/50987505/how-to-get-error-reason-when-curl-easy-init-fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thanks, yes, that seems helpful!
sweet, looking forward to see this once it is working :) |
@vigsterkr I copied a few things from deadbeef to add RapidJSON as a dependency |
saw it 😼 |
src/shogun/CMakeLists.txt
Outdated
CONFIG_FLAG HAVE_XML) | ||
# RapidJSON | ||
include(external/RapidJSON) | ||
SHOGUN_INCLUDE_DIRS(SCOPE PUBLIC ${RAPIDJSON_INCLUDE_DIR}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vigsterkr is it ok to have RapidJSON as PUBLIC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it was like this in the other PR, right? i mean currently i include the RapidJSON only in .cpp so private should be fine.... but maybe/probably that caused me troubles with unit tests ...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was private in the other PR, but swig needs the RapidJSON header files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? now i understand why :)
src/shogun/io/OpenMLFlow.h
Outdated
#include <shogun/io/SGIO.h> | ||
|
||
#include <curl/curl.h> | ||
#include <rapidjson/document.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would avoid this by all means :)
src/shogun/io/OpenMLFlow.h
Outdated
components_type m_components; | ||
|
||
#ifndef SWIG | ||
static void check_flow_response(rapidjson::Document& doc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it really need to be part of the class, meaning can't you just have this function in the implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I guess I can move to free function and avoid exposing RapidJSON to swig :)
src/shogun/io/OpenMLFlow.h
Outdated
#ifndef SWIG | ||
static void check_flow_response(rapidjson::Document& doc); | ||
|
||
static SG_FORCED_INLINE void emplace_string_to_map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above...
src/shogun/io/OpenMLFlow.h
Outdated
param_dict.emplace(name, ""); | ||
} | ||
|
||
static SG_FORCED_INLINE void emplace_string_to_map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
4a6f93b
to
45ac04e
Compare
df8809d
to
ea27c0a
Compare
394b4bd
to
8a98439
Compare
I have a local version mostly working (with binary classification tasks at least). There are some memory issues but they will be fixed once we switch to smart pointers. |
What are 9602 and 4423? |
Those are the IDs of a flow and a task that I used with the openml-python extension I wrote and i know they work |
7842b61
to
54b2936
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
yes, let's keep it :) |
adds OpenML natively to shogun.
TODO: