Refactoring Code Structure

[gfrances]

@miquelramirez , I'm trying to figure out which of the novelty features here could/should be moved to a new location modules/hybrid that I'm creaing (don't look for that, only in local ATM). The very name modules/hybrid is tentative, but essentially I wanted to put there all code related with soplex. Feel free to suggest a better name, or to suggest splitting that into two or more conceptually differentiated modules, and I'll do that. In any case, for instance the elliptical_2d.* code will end up there, because it uses soplex, etc. etc.
I will probably have some "feature wrapper" code in modules/hybrid that conditionally includes soplex, and throws a runtime error if both

• the use_soplex flag was not used during compilation, and
• the user is trying to use a soplex-related option in the command line, such as e.g. features.elliptical_2d, etc.

Thoughts?

[gfrances]

BTW perhaps an fs0.hybrid namespace would also make sense?

[gfrances]

Bonus question: The G0Heuristic is used anywhere? Just trying to grep my way around it but can't find its use...

[miquelramirez]

Hola @gfrances,

1. what you propose makes sense, and yes, certainly those features depend on a specific representation of the variables.

2. there's already a fs0::hybrid namespace, but perhaps I have missed one or two classes from being in there.

3. soplex is right now only used with the purpose you correctly identify, but it could be used in the future to implement the CRPG (for very specific kinds of planning problems) or to check for existentially quantified CSPs which are formulated over a constraint language that consists exclusively of linear inequalities. So I wouldn't make things too hard for those future and likely uses.

4. More generally, rather than being coupled to soplex we may want to look into the Open Solver Interface, that helps abstracting LP and MILP solvers.

5. G0Heuristic, like L0Heuristic are ongoing "experimental" concepts, probably from G0Heuristic we will eventually branch off something to do with Florian.

Un abrazo!

[gfrances]

Thanks Miquel, good info. Related question: I'm seeing here that the SBFWS driver now has some hybrid::L2Norm pointer. This effectively means (following the include dependency chain) that I need to have soplex to use the driver, which is not too nice. In general, I can put in lp.hxx (or wherever relevant) something similar to

#ifdef FS_USE_SOPLEX
        #include <soplex.hxx>
#else 
        static_assert(false, "...");
#endif

This however does not solve the situation here, since in the code as it is now will always end up including soplex, irrespective of whether the runtime configuration dictates its use or not...
The point is: I'd rather not clutter the whole codebase with #ifdefs each time e.g. something like the _l2_norm attribute above is declared/used/mentioned. Can you envision a better way to do so?

[miquelramirez]

Hello @gfrances ,

certainly, the code needs to be decluttered a bit. There is no reason for hybrid::L2Norm and L0Heuristic to be used directly. If you look through the code of SBFWSHeuristic you'll see that they're being used conditionally, depending on the option selected to compute the R-set. We need to have a factory of implementations for the R-Set, which resolve into relatively simple function objects with signature

node -> unsigned

wrapping the call and the dependency with those norms.

Factorising the computation of the R-sets if something we would want to do anyways (so we can use intervals/sets from the CRPG heuristic, landmarks, etc.)

Cheers,

Miquel.

[gfrances]

Another quick one, @miquelramirez : what was the purpose of duplicating iw_run from mv_iw_run? You think it'd be hard to use one single (refactored) class for both use cases?

[miquelramirez]

Hello @gfrances ,

I didn't want to break inadvertently the implementation of IWRun so I pretty much forked the thing, with the hope that we would find the time to look at how each branch had evolved and see what (if anything) was actually different 🥇

[gfrances]

haha ok, but you mean that it could well be that there is no major incompatibility?

[gfrances]

The sheer number of template parameters we have on every class is starting to give me serious headaches...

[miquelramirez]

The mv_iw_run variant is meant to work over multi-valued features. It's been over a month since the last time I looked through those codes. I can take a quick look tomorrow.

Let's say that the profusion of template parameters was for sure one of the reasons why I duplicated stuff. Compile time polymorphism, like const, sometimes behaves in perverse ways.

[gfrances]

So if I slowly started making some template parameters disappear by using a bit more of runtime polymorphism, trading off some (hopefully small) runtime performance by some peace of mind, would I have your blessing?

[miquelramirez]

Actually, an old colleague of mine and very experienced C++ programmer referred to both of them, hilariously, as the only STDs programmers' usually get.

[miquelramirez]

So if I slowly started making some template parameters disappear by using a bit more of runtime polymorphism, trading off some (hopefully small) runtime performance by some peace of mind, would I have your blessing?

Yes, please

[gfrances]

The mv_iw_run variant is meant to work over multi-valued features. It's been over a month since the last time I looked through those codes. I can take a quick look tomorrow.

It'd be good if I have your opinion on the main difficulties to merge back both variants - but after no more than a quick 5-minute look, no worries for that

[miquelramirez]

Will try to do so over the next few days @gfrances :)

[miquelramirez]

Hi there @gfrances ,

some notes on this:

• IWRunNode vs MultiValuedRunNode: The biggest differences between the two classes lie in code which was commented out and I deleted. Other than that, the real difference (and subtle) I see is that the attribute _nov_2_pairs needs to be defined differently in either class. For the former, is a table of pairs of AtomIdx, in the latter a pair of Width1Tuple a type which is imported from lapkt::novelty and defined around the language type we represent our datums (FSFeatureValueT)..

• SimulationEvaluator vs MultiValuedSimulationEvaluator: the major difference between the two classes is in the implementation of the method reached_atoms(). In both classes it is defined as const, but it has different return types: std::vector<bool> vs. std::vector<Width1Tuple>. This is important: the former assumes datums are booleans, the latter assumes datums to be FSFeatureValueT. Also a convenience accessor to the set of features associated with the evaluator has been added to the interface of MultiValuedSimulationEvaluator.

• IWRun::Config vs MultiValuedIWRun::Config: the attribute mark_negative has been removed from the latter as it doesn't make sense in the context of the second class. Three new attributes have been added to the latter, _R_file and _log_search activate the code that allows to load the R set from a file, and the code that logs the IW search tree into a JSON document. _filter_R_set is no longer used: I did some experimentation with doing some "post processing" on the R-set, without much success.

• MultiValuedIWRun::DeactivateZCC class added to track option to ignore state constraints during the IW(k) lookahead. The term "zero crossings" refers to allowing IW to shoot through the "holes" in the state space created by state constraints.

• IWRun vs MultiValuedIWRun: quite a few changes

  • The attribute _visited has been added to MultiValuedIWRun to store the set of states visited during the lookahead.
  • MultiValuedIWRun::mark_tuples_in_path_to_subgoal(): Important changes here, as first, datums are no longer boolean, and rather than accessing state variables directly, we use the feature set in the evaluator.

• MultiValuedIWRun::mark_all_tuples_in_path_to_subgoal: Analogous changes to the above.

• MultiValuedIWRun::compute_R_all(): Method has been disabled with an exception.

• MultiValuedIWRun::compute_R(): Added code to load R-set from file.

• MultiValuedIWRun::compute_plain_RG2(): Added check for goal_directed flag, if false, extract_R_1 is called.

• MultiValuedIWRun::compute_plain_R1(): Changed return type to match that of the datums.

• MultiValuedIWRun::load_R_set(): Loads the R set from a file.

• MultiValuedIWRun::filter_R_set(): Experiments with R-set filtering, no longer used.

• MultiValuedIWRun::compute_coupled_features(): Added toIWRun because of static polymorphism. Method used in experiments filtering R-set, no longer used.

• MultiValuedIWRun::dump_R_set(): Code to dump the R-set to a file.

• MultiValuedIWRun::extract_R_1(): Changed return type to match that of datum.

• MultiValuedIWRun::compute_R_g_prime(): $R_{all}$ fallback disabled.

• MultiValuedIWRun::compute_adaptive_R(): Changed return type to match that of datum.

• MultiValuedIWRun::extract_R_G(): Added code to dump R set to file, changed return type to match that of the datum.

• MultiValuedIWRun::extract_R_G_1(): Idem as MultiValuedIWRun::extract_R_G().

• ```MultiValuedIWRun::run()``: added code to check if IW(k) lookahead needs to respect state constraints, uses this info to influence the behaviour of the applicable action iterators.

• ```MultiValuedIWRun::report()``: added code to store the search tree that follows from IW(k) into a file.

Summary

The filtering of R sets and its associated methods need to be moved out of the class. I would like to have them somewhere, as a snippet of some sort and a note, since the idea of post-processing R-sets may have some potential.

The rest of the changes are related to switching the underlying datum used to compute w(s) from bool to FSFeatureValueT (which is unsigned), and that we need to use the features to obtain the relevant valuations.

[miquelramirez]

I hope this helps!

[gfrances]

@miquelramirez ! I have finished a slight refactoring of the code to start really decoupling the modules, so that e.g. I can compile and run the planner without soplex, etc. (of course NOT run the LP-based parts of the planner). This is done in commit ed16894.
The relevant parts are:

• I'm moving hybrid-related stuff to this directory.
• I'm using the same macro-based trick as in FD, see e.g. here. It's ugly, but not that I can think of too many cleaner alternatives. A good one that I have in mind would be to use more clearly-defined stubs, i.e.with a clear interface, and plug (through a factory method, etc.) the appropriate "real" object when an LP solver is available, and a "fake" object that just throws a runtime error when used, when no LP solver is available. What I did now is not too far from that though.
• This is all very unstable yet, but I'm thinking on changing a bit the layout of the code to have a clearer module-based layout, and eventually adopt the same kind of structure that we wanted for LAPKT. So for instance we might have directories src/core, src\hybrid, src\gecode, etc. etc. which would make it easier to eventually use submodules, etc., if we deemed that necessary.

G.

[miquelramirez]

Great to see this going forward @gfrances :)

Some thoughts inline:

• Looks good to me. Moving the directory dynamics is something I would rather do when we're done with ICAPS, as I have been doing a few important modifications, so we can merge beta5 into beta6 easily.
• We perhaps want to have proper includes, all of them based on fs? Like

#include <fs/core/problem.hxx>

that would be a robust approach when deploying fs as a library.

• We shouldn't use relative includes like .../base.hxx they're very fragile.
• The scheme is actually less restrictive than what I had in mind, which is having this

#ifndef FS_XXXX
#error "You need to compile the planner with XXXX support in order to use this method.""
#endif

on each translation unit and header that depends on XXXX. The #error macro would be generating these errors at compile time. Checking for dependencies at run time and this on a per-method/per-attribute basis sounds useful, if the class / interface remains meaningful or useful without those components. I am not sure how often this happens to us, other than in very high level interfaces such as the factories for constructing heuristics, etc.

• I will generate a pull request into beta5 today so you can see what changes I have been making so far, so we can merge before the branches become unmergeable.

[gfrances]

Sure, if you do that PR I will try to merge that right away. The "proper includes" suggestion sounds heavenly. Agree with the relative includes, I've just started using CLion, which by the way I'm finding far better than Kdevelop, but I need to see how to make it generate absolute includes.

[miquelramirez]

Doing the PR in a few minutes!

[miquelramirez]

PR #92 is up

[gfrances]

Ok, I merged your PR into beta5. I understand that you're still actively making changes in your branch of the code? Are you planning to make changes to the "core" part as well? I'm eager to push this forward, as I have other things that need to build upon some working version of the code, which I don't really have at the moment, or not too reliably. Let me know what your plans regarding code changes are, and let's see how we can better do this :-)

Thanks!

[miquelramirez]

Hello there @gfrances ,

What I can assure you that within the next 24 hours I am not planning on doing any further changes on the framework - I will be working on the satellite repos, trying to gather more experimental observations. So no major changes other than bugfixes.

My suggestion is that you go and finish the refactoring as quickly as possible, so I can then bring beta6 to my fork, and assess how much stuff gets broken by the changes.

Also, as long as no filenames are changed, I should be able to bring over any emergency changes. Keeping a changelog of sorts detailing what was moved where and how includes need to change would also be very helpful.

If something gets broken in a subtle way - i.e. BFWS/IW etc. - then I will fall back to beta5 and see through this transition after ICAPS.

Cheers,

Miquel.

[gfrances]

Sounds good - but don't try to merge anything into your fork until after the deadline!
I'll keep you informed. Let's see if we can move forward with this :-)

[gfrances]

BTW, which name do you suggest for the "hybrid" module? "Hybrid" works well? Something different?

[miquelramirez]

Okay, @gfrances if you say don't merge I won't merge, no worries.

hybrid will work - rather generic but it is short and to the point.

As I said before with proper documentation explaining the changes in some detail, things will be way easier. At least one can go and sort out the big stuff and then worry about the bits that always fall through the cracks.

[gfrances]

Note: I'm starting to document the changes in file beta7-dev-notes.md

[gfrances]

In particular, let me highlight this change:

The rapidjson library is no longer under the src directory, but rather
on a first-level vendor directory (lib was already taken!).
Additionally, the code is no longer versioned on our repo, but instead we fetch it using submodules.
This means that you need to git submodule update whenever a change in the rapidjson library
is factored into the main FS project - which shouldn't happen too often.

I take this as a test... with things such as rapidjson I guess using submodules is perfect.
I'm not so sure however that submodules are the right tool for the main development of the planner,
e.g. if we wanted to keep the different modules on different repos, as in LAPKT. What do you think?
That would require some changes, of course, but hopefully not too many. But with all of the build scripts... not sure how flexible submodules are to reuse too many of them?

[miquelramirez]

The rapidjson library is no longer under the src directory, but rather
on a first-level vendor directory (lib was already taken!).
Additionally, the code is no longer versioned on our repo, but instead we fetch it using submodules.
This means that you need to git submodule update whenever a change in the rapidjson library
is factored into the main FS project - which shouldn't happen too often.

That basically automates the process of synching with rapidjson - I was doing the same by hand.

I'm not so sure however that submodules are the right tool for the main development of the planner,
e.g. if we wanted to keep the different modules on different repos, as in LAPKT. What do you think?
That would require some changes, of course, but hopefully not too many. But with all of the build scripts... not sure how flexible submodules are to reuse too many of them?

I don't think so. If we want to do spin offs from fs for research (i.e. developing planners for specific settings etc.) we can do so using fs as a library. For an example, you can check out the repo I think I already shared with you miquelramirez/hybrid-mpc-planner.

[gfrances]

You mean you don't think that submodules are the way to go, right?
I agree that FS as a lib is nice for specializing planners for particular uses, etc., as you say (would be a great thing to have Jonathan's CTMP project as a separate repo making use of the lib, instead of a branch that mixes all sorts of domain-dependent code in the main FS repo, but well... too late for that probably.

My point however is whether the same idea is also good for more complex settings such as hybrid planning itself. Could we for instance separate the hybrid planning part of the code into a different repo that imports and makes use of the FS library in the same manner? That'd be a good thing to have.

But then we could further modularize the planner, as we're starting to do, and have all things that depende on a CSP solver as a separate module. And so on. But the development of all of this modules is (at least at the moment) quite interlocked - i.e. changes are likely to occur simultaneously accross the different modules. Hence I was thinking that keeping everything separate might be a nightmare, as it'll be crucial to keep track of which commits in each modules are compatible with which commits in the other modules, etc. That's where git submodules could prove useful, although I'm a bit skeptical about their ease of use...?

P.D: By the way, very cool hybrid planner, eventually we'll get to the point where FS offers a clear python interface which includes programmatic creation of problems etc.... soon enough! :-)

[gfrances]

I am closing this big and messy issue here, as we're mostly done with it. I opened #96 for the only part which remains to be done.