ivapylibs / puzzle_solver

1 stars 2 forks source link

Code: puzzle.builder.gridded #17

Closed pv33 closed 3 years ago

pv33 commented 3 years ago

The gridded class has been pseudocoded

The interlocking class was incomplete. The good thing is that the organized nature of a gridded puzle permits us to overload the processing operator and avoid needing to invoke the superclass versions (well at least avoid directly calling the interlocking version though it might call some of its member functions. maybe or maybe not, depends on how things get written in the end).

note: not yet committed and pushed. should be by the time you get to this code (will remove note when pushed).

Depends on #16

Uio96 commented 3 years ago

Just have a quick check on the 15 image https://github.com/ivapylibs/puzzle_solver/blob/master/puzzle/testing/data/puzzle_15p_123rf.png. It fails because fromLayer.py could not work on this touching case. Will have to upgrade it first. Since it is the same problem with other testing scripts, e.g., adjacent, etc. So I only update the tasks of this issue.

pv33 commented 3 years ago

@Uio96 Actually, I would say that you have to modify the detector. As a trackPointer, fromLayer is totally legit if given a correct binary mask. What I noted in the discussion today is that you have to make sure to generate a correct mask from that image. That image is not the true mask, but a sketch. In this instance, you would need to create the proper detector to go from sketch to binary puzzle piece regions. After that, everything will work just fine.

It might be that a new static member function is needed called buildFrom_ImageAndSketch or something like that.

Seems like your conception of the classes is still quite limiting. You have yet to really understand how to effectively leverage them.

Uio96 commented 3 years ago

I am not quite

@Uio96 Actually, I would say that you have to modify the detector. As a trackPointer, fromLayer is totally legit if given a correct binary mask. What I noted in the discussion today is that you have to make sure to generate a correct mask from that image. That image is not the true mask, but a sketch. In this instance, you would need to create the proper detector to go from sketch to binary puzzle piece regions. After that, everything will work just fine.

It might be that a new static member function is needed called buildFrom_ImageAndSketch or something like that.

Seems like your conception of the classes is still quite limiting. You have yet to really understand how to effectively leverage them.

Yep. The problem is with the detector. I will upgrade it this week.

pv33 commented 3 years ago

@Uio96 Where by "upgrade" you mean "create a new detector class instance that can perform the sketch to mask processing." It should not be in the detector code repo but in the puzzle repository. Where doesn't matter since we can always `git mv if it is improper.

Uio96 commented 3 years ago

Grid coordinates: [[4. 3. 1. 0. 2. 4. 3. 2. 1. 0. 4. 3. 2. 1. 0.] [2. 2. 2. 2. 2. 1. 1. 1. 1. 1. 0. 0. 0. 0. 0.]]

Have been finished. See https://github.com/ivapylibs/puzzle_solver/blob/master/puzzle/builder/testing/basic07_gridded.py for details.

pv33 commented 3 years ago

basic05_gridded.py does not run because several imports are not coded into the python scripts for detector.inImage.

I suspect that you are still relying on your IDE to create the environment and are not testing on the command line, which is how many things will be run in the end, or so I anticipate based on using ROS. Not acceptable. I also suspect that many more test scripts do not run because of this problem.

Besides the test script above, all test scripts should be run from the command line. More so, they should have

!/usr/bin/python3

as the first line and be made executable. I should be able to run them from the command line by simply invoking them as an executable. For the basic05 test script, that wuold be:

./basic05_gridded.py

and it should work. In fact, it should run from any directory. From the base puzzle solver, I should be able to run:

./puzzle/builder/testing/basic05_gridded.py

and it will work. Most likely the above is not going to hold true for many scripts that rely on loading information unless you followed the scripts I wrote that do path retrieval.

pv33 commented 3 years ago

@Uio96 My bad on detector.inImage. I failed to pull the latest code. It was using a bad, partially implemented version.

I updated basic01_arrangement.py to be executable and it worked. Moving up to basic05 now. Should be good.

pv33 commented 3 years ago

@Uio96 For the saving and loading, why was a dataclass used? From this thread, it looks like you can just put the data into a tuple, or a named tuple.

It's not a big deal, just curious.

Ultimately, the buildFromFile factory method doesn't need the class, so it seems like overkill to create one when there is a simpler alternative. Trying to understand your python coding logic. This seems like the opposite scenario to Yiye's parameter structure.

Uio96 commented 3 years ago

@Uio96 For the saving and loading, why was a dataclass used? From this thread, it looks like you can just put the data into a tuple, or a named tuple.

It's not a big deal, just curious.

Ultimately, the buildFromFile factory method doesn't need the class, so it seems like overkill to create one when there is a simpler alternative. Trying to understand your python coding logic. This seems like the opposite scenario to Yiye's parameter structure.

Previously, you mentioned that you wanted a struct instance similar to the one in Matlab. I think dataclass is a good option for that.

pv33 commented 3 years ago

Success! All 7 scripts execute successfully.

pv33 commented 3 years ago

@Uio96 Yes, but that is because of the organized nature of the parameters structure. Saving variables into a file doesn't quite rise to the same level. In that instance, named tuples work fine. A file is just a bag of elements. The elements just need a name to retrieve.

The nature of the two cases are quite different even though the base or procedural functionality is similar. Code should be structured around the nature of the operation, not the functionality of the operation. Not asking to change since it doesn't matter, just that it is overkill.

Asking for something in one context doesn't mean it is needed for all contexts. Put slightly differently, just because one approach made sense in one context, doesn't mean it necessarily applies to all similar contexts. There might be considerations that change how to interpret those similar contexts. You are making dataclass into a hammer and treating other things as nails when they need not be.

pv33 commented 3 years ago

@Uio96 Accepting current code and will close. Long term, it might be that the inconsistent enumeration of pieces will need to be fixed. Sometimes one is out of order relative to row-wise or column-wise labeling.