pass instances #34
pass instances #34
Conversation
Codecov Report
@@ Coverage Diff @@
## main #34 +/- ##
==========================================
- Coverage 92.11% 91.68% -0.43%
==========================================
Files 12 12
Lines 431 469 +38
==========================================
+ Hits 397 430 +33
- Misses 34 39 +5
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
DE0CH
force-pushed
the
pass-instance-pre
branch
from
0d2ba37 to
5d18b8a
Compare
| q = Queue() | ||
| scenario = dict( | ||
| instances = [q], |
There was a problem hiding this comment.
Why would an instance be a Queue? What is this testing? Could you add a testcase that is more realistic, like instances being a list of functions that need to me minimized?
There was a problem hiding this comment.
It's because Queue is not serializable and can only work if the queue on the other hand shares the same memory address.
There was a problem hiding this comment.
But why pass it in "instances" and not via other way? You are using "instances" for something that is not its purpose.
Does this need to be part of the scenario?
There was a problem hiding this comment.
I guess it doesn't need to be. But I think it's nice if there are multiple ways to pass stuff around.
| logFile = "", | ||
| seed = 123, | ||
| instanceObjectSerializer = lambda x: 'hello world' |
There was a problem hiding this comment.
What is this doing? Could you document it somewhere?
Irace is extremely well documented and most of its documentation currently applies to iracepy. See: https://mlopez-ibanez.github.io/irace/ and https://mlopez-ibanez.github.io/irace/irace-package.pdf
| self.context.update({ | ||
| 'py_instances': self.scenario['instances'], | ||
| }) | ||
| self.scenario['instances'] = StrVector(list(map(lambda x: json.dumps(x, skipkeys=True, default=self.scenario.get('instanceObjectSerializer', lambda x: '<not serializable>')), self.scenario['instances']))) |
There was a problem hiding this comment.
Does this mean that the serializer is applied to strings and integers? Isn't that a waste?
Also, could you document with a comment what this line is doing? It seems to replace the instance with the string '<not serializable>' unless the user provides an 'instanceObjectSerializer'. Is that the case? If so, how can the user serialize their instances? It is unclear to me how all this works and will be used in practice.
A couple of examples using for example:
- Functions that need to be optimized by https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.minimize.html#scipy.optimize.minimize
- Matrices that are to be optimized by https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.quadratic_assignment.html#scipy.optimize.quadratic_assignment
|
Ok. I will make the changes. But can you first decide if you will accept PR #22? It’s hard for me to maintain two separate branches. |
It is a lot of new code whose purpose is not completely clear to me. One problem is that we have only 1 scenario in iracepy, which is the dual-annealing.py. It would be useful for testing and for development to have more varied scenarios. It is also not an alternative to the current code (so if it stops working or prevents more common uses of irace, we could delete it or ignore it). Instead, it completely replaces the current code, which means that getting back to the iracepy we have now will be a lot of work. Having two branches, one more experimental and another stable seems like a good idea to me. |
|
But if you intend to work on iracepy in the long term and not just for the current project, I'm happy to make a fork of the current state as my personal "stable" branch, and then you can take over the main repository. I don't want to stop you from making progress. |
Can we fork a branch called 1.0.x where we keep everything stable and use main for more experimental stuff while aiming for 2.0?
Thanks. I am probably not going to work on it in the long term. Though, I have some questions about how I should structure my code for the current project. If I start depending on features introduced in PRs that are eventually rejected, then it would mean a lot of refactoring for the project code. So should I just not write new features into |
This is #30 with main as the base.
Closes #29.