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

Dictionary “order” is relevant when defining condition parameters #113

Closed
dpaetzel opened this issue Dec 1, 2023 · 2 comments · Fixed by #114
Closed

Dictionary “order” is relevant when defining condition parameters #113

dpaetzel opened this issue Dec 1, 2023 · 2 comments · Fixed by #114
Labels

Comments

@dpaetzel
Copy link
Contributor

dpaetzel commented Dec 1, 2023

This is somewhat related to #111; I think the fix for that may have resulted in a new bug: It is now suddenly important to provide the "type" key as the first item in the dictionary. However typical Python dictionaries do not have a way of making something the first item reliably because they're unordered. The following does not work in most cases:

import numpy as np
import xcsf
import json

model = xcsf.XCS(max_trials=10, condition={"args": {"spread_min": 0.5}, "type" : "hyperrectangle_csr"})

pop = json.loads(model.json())["classifiers"]

spreads = [rule["condition"]["spread"][0] for rule in pop]
print(np.min(spreads))

Note that the last three lines of code are not even reached because the whole Python process gets killed by the No condition type has been specified: cannot set params error. Is that intended? (This may be a separate issue, though.)

@rpreen
Copy link
Member

rpreen commented Dec 1, 2023

Ah, yeah I'll look at the ordering issue.

Yes, I think it should terminate running if there is an obvious parameter error to make sure it's running exactly as requested (normally throwing an exception would be best, but the current way the C library is built and linked means getting that back up to Python needs some thought - it does do it in some places - and needs to be done in multiple locations not just here) -- I have created issue #115 to deal with this.

@dpaetzel
Copy link
Contributor Author

dpaetzel commented Dec 6, 2023

Thank you for the quick fix! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants