Improve the code and add the tests by lgarridobsq · Pull Request #8 · BLSQ/openhexa-ds-developments

lgarridobsq · 2026-03-26T09:49:57Z

@EstebanMontandon ,

I improved the code a bit
I added the tests
I tested in my computer and i could use the funcitons once I downloaded the package

Tell me what you htink!

EstebanMontandon

Nice work, this is finally taking shape :') congrats

pyramid_matching/pyramid_matcher/pyramid_matcher.py

pyramid_matching/README.md

EstebanMontandon · 2026-03-30T09:09:22Z

pyramid_matching/pyramid_matcher/pyramid_matcher.py

+        self.prefix_candidate_data: str = "candidate_"
+        self.prefix_reference_data: str = "reference_"
+
+    def _init_logger(self, logger: logging.Logger | None) -> None:


I'm starting to doubt a bit about the logging. The pyramid matcher returns a exhaustive result summary, probably we don't need to additionally provide a logging. We can always decide how to handle any situation based on the outputs. We could remove this, what do you think?

Mmmmmmmmmm
I am not sure! I have two worries:

(sometimes) the levels and (always) the attributes are detected by the algorithm. I think its nice if they get printed

The function can take a long time (specially if we are matching a lot of level_5's) -- I think its nice to have an indication of what is happening.

pyramid_matching/pyramid_matcher/pyramid_matcher.py

pyramid_matching/README.md

EstebanMontandon · 2026-03-30T09:52:23Z

pyramid_matching/README.md

+- `repeated_level_*` columns indicate whether the match for that level is repeated (i.e., if the same reference level is matched to multiple candidate levels).
+
+
+The output `matched_data_simplified` will be:


This is very good, as a User I want to get quick simple mapping table based on matching columns that will allow me to perform my task.

Just to clarify my thoughts about the algorithm: as we use parameters names reference and candidate pyramids, how is the output table structured relative to the reference input?

As a user, I would expect:

The output is essentially a copy of the reference pyramid

Each (unique) organization unit from reference is paired with a candidate at the matching level

Is expected that some rows would have None values in the "candidate_" prefixed columns (for unpaired units)

Is this correct?

mmmmmmmmmm no

The output is essentially a copy of the reference pyramid --> yes

Each (unique) organization unit from reference is paired with a candidate at the matching level --> yes

Is expected that some rows would have None values in the "candidate" prefixed columns (for unpaired units)_ --> no

The output does look like what you said -- a copy of the candidate pyramid with the relevant reference columns attached to it.

Nevertheless, there are no None's on the candidate columns... If a row is not matched, it is not in the output. (I think this makes sense -- if I have not matched a row, I do not want it...)

(You can see the not-matched things on the not_matched outputs)

What do you think?

lgarridobsq

Hola!

I changed a couple of things and answered your comments...

if you want, approve and we merge ⭐

pyramid_matching/pyramid_matcher/pyramid_matcher.py

lgarridobsq · 2026-03-31T15:42:34Z

pyramid_matching/pyramid_matcher/pyramid_matcher.py

+        self.prefix_candidate_data: str = "candidate_"
+        self.prefix_reference_data: str = "reference_"
+
+    def _init_logger(self, logger: logging.Logger | None) -> None:


Mmmmmmmmmm
I am not sure! I have two worries:

(sometimes) the levels and (always) the attributes are detected by the algorithm. I think its nice if they get printed

The function can take a long time (specially if we are matching a lot of level_5's) -- I think its nice to have an indication of what is happening.

lgarridobsq · 2026-03-31T16:00:20Z

pyramid_matching/README.md

+- `repeated_level_*` columns indicate whether the match for that level is repeated (i.e., if the same reference level is matched to multiple candidate levels).
+
+
+The output `matched_data_simplified` will be:


mmmmmmmmmm no

The output is essentially a copy of the reference pyramid --> yes

Each (unique) organization unit from reference is paired with a candidate at the matching level --> yes

Is expected that some rows would have None values in the "candidate" prefixed columns (for unpaired units)_ --> no

The output does look like what you said -- a copy of the candidate pyramid with the relevant reference columns attached to it.

Nevertheless, there are no None's on the candidate columns... If a row is not matched, it is not in the output. (I think this makes sense -- if I have not matched a row, I do not want it...)

(You can see the not-matched things on the not_matched outputs)

What do you think?

pyramid_matching/README.md

lgarridobsq requested a review from Copilot March 26, 2026 09:50

Copilot started reviewing on behalf of lgarridobsq March 26, 2026 09:50 View session

This comment was marked as resolved.

Sign in to view

lgarridobsq force-pushed the DSDE-218_finalize_pyramid_matching branch 2 times, most recently from 0321bca to ff67e06 Compare March 26, 2026 10:56

lgarridobsq requested a review from Copilot March 26, 2026 10:56

Copilot started reviewing on behalf of lgarridobsq March 26, 2026 10:56 View session

This comment was marked as resolved.

Sign in to view

lgarridobsq force-pushed the DSDE-218_finalize_pyramid_matching branch from ff67e06 to 07a74c0 Compare March 26, 2026 11:11

lgarridobsq requested a review from Copilot March 26, 2026 11:11

Copilot started reviewing on behalf of lgarridobsq March 26, 2026 11:12 View session

This comment was marked as resolved.

Sign in to view

lgarridobsq force-pushed the DSDE-218_finalize_pyramid_matching branch from 07a74c0 to 8ab5335 Compare March 26, 2026 11:26

lgarridobsq requested a review from Copilot March 26, 2026 11:26

Copilot started reviewing on behalf of lgarridobsq March 26, 2026 11:26 View session

This comment was marked as outdated.

Sign in to view

improve the code and add the tests

61f172f

lgarridobsq force-pushed the DSDE-218_finalize_pyramid_matching branch from 8ab5335 to 61f172f Compare March 26, 2026 13:06

lgarridobsq requested a review from EstebanMontandon March 26, 2026 13:18

EstebanMontandon reviewed Mar 30, 2026

View reviewed changes

lgarridobsq commented Mar 31, 2026

View reviewed changes

changes after Esteban's review

96ba4f7

lgarridobsq force-pushed the DSDE-218_finalize_pyramid_matching branch from ca1fc3b to 96ba4f7 Compare March 31, 2026 16:40

		- `repeated_level_*` columns indicate whether the match for that level is repeated (i.e., if the same reference level is matched to multiple candidate levels).


		The output `matched_data_simplified` will be:

Conversation

lgarridobsq commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

EstebanMontandon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EstebanMontandon Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

lgarridobsq Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EstebanMontandon Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

lgarridobsq Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

lgarridobsq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgarridobsq Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

lgarridobsq Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lgarridobsq commented Mar 26, 2026 •

edited

Loading