Data_Engine: Handling of nested request issues fixed #2311

pawelbaran · 2021-02-15T21:30:08Z

Issues addressed by this PR

Closes #2310

Test files

This is an issue-specific file. However, all other files from this folder should still work correctly.

Additional comments

…sted inside LogicalNotRequest

…the same type as splitting type fixed

adecler

I am not a big fan of the changes made in this PR. The issue you are trying to fixed is cause by AllRequestsOfType not by SplitRequestTreeByType.
I got the test file you provided working by making the following changes on the master instead:

pawelbaran · 2021-02-18T11:22:13Z

I am not a big fan of the changes made in this PR. The issue you are trying to fixed is cause by AllRequestsOfType not by SplitRequestTreeByType.

Not really @adecler: the issue consists of 3 sub-issues, your suggested change seems to be fixing only the 1st from what I can see. The other two still require intervention (and therefore I am changing three files instead of one).

From what I can deduct from reading the code, the change proposed by you in AllRequestsOfType will provide the same behaviour as the change introduced in this PR (you moved the null check before the recursive call).

By the way, I agree that the code looks pretty convoluted as for a relatively simple tree handling problem, but I could not make it any better due to the fact that some ILogicalRequests store children in a list, while LogicalNotRequest has a single child property - that is the origin of the problem. I will be more than happy to simplify the code, so further improvement suggestions are more than welcome 👍

adecler · 2021-02-19T01:20:20Z

Not really @adecler: the issue consists of 3 sub-issues, your suggested change seems to be fixing only the 1st from what I can see. The other two still require intervention (and therefore I am changing three files instead of one).

From what I can deduct from reading the code, the change proposed by you in AllRequestsOfType will provide the same behaviour as the change introduced in this PR (you moved the null check before the recursive call).

It solved the two red components in the example file for me so didn't look further (difficult to debug something that is already working). Then consider this change request limited to the SplitRequestTreeByType file. Fixes should be made inside the method causing problem, not in calling methods. Otherwise, the problem will reappear every time you have a new method calling it.

By the way, I agree that the code looks pretty convoluted as for a relatively simple tree handling problem, but I could not make it any better due to the fact that some ILogicalRequests store children in a list, while LogicalNotRequest has a single child property - that is the origin of the problem. I will be more than happy to simplify the code, so further improvement suggestions are more than welcome 👍

Yes, I am not a big fan of having to constantly check if an Irequest is an ILogicalRequest. What if you add more types of requests ? Will you have to constantly add more checks everywhere IRequests are being used ? This is why it is important to keep the needs for those check as local as possible. AllRequestsOfType is playing the role of the getter so you could have a similar method that would play the role of the setter. So SimplifyRequestTree wouldn't have to do those checks itself.

pawelbaran · 2021-02-19T08:50:44Z

Not really @adecler: the issue consists of 3 sub-issues, your suggested change seems to be fixing only the 1st from what I can see. The other two still require intervention (and therefore I am changing three files instead of one).
From what I can deduct from reading the code, the change proposed by you in AllRequestsOfType will provide the same behaviour as the change introduced in this PR (you moved the null check before the recursive call).

It solved the two red components in the example file for me so didn't look further (difficult to debug something that is already working). Then consider this change request limited to the SplitRequestTreeByType file. Fixes should be made inside the method causing problem, not in calling methods. Otherwise, the problem will reappear every time you have a new method calling it.

Yeah, and the fixes are made inside the methods causing problems, nowhere else - there is simply 3 methods causing different problems, not one (see the issue). And the problem is not only the component turning red, it is also about the output being wrong, that is why there is a few scripts, not one.

By the way, I agree that the code looks pretty convoluted as for a relatively simple tree handling problem, but I could not make it any better due to the fact that some ILogicalRequests store children in a list, while LogicalNotRequest has a single child property - that is the origin of the problem. I will be more than happy to simplify the code, so further improvement suggestions are more than welcome 👍

Yes, I am not a big fan of having to constantly check if an Irequest is an ILogicalRequest. What if you add more types of requests ? Will you have to constantly add more checks everywhere IRequests are being used ? This is why it is important to keep the needs for those check as local as possible. AllRequestsOfType is playing the role of the getter so you could have a similar method that would play the role of the setter. So SimplifyRequestTree wouldn't have to do those checks itself.

Actually, the problem is not only the existence of ILogicalRequest, but also LogicalNotRequest that stores children in a different structure. This mismatch makes it really hard to make the code more simple (and I am afraid a setter method will not help much here).

To push this PR forward, I would opt for treating it as a bugfix and no more than that, not to keep it hanging. Afterwards, I am happy to raise an issue to discuss simplification and possible wider refactoring of the IRequests, which I believe is beyond the scope of this PR.

adecler · 2021-02-19T08:57:21Z

To push this PR forward, I would opt for treating it as a bugfix and no more than that, not to keep it hanging. Afterwards, I am happy to raise an issue to discuss simplification and possible wider refactoring of the IRequests, which I believe is beyond the scope of this PR.

Agreed, I just provided a suggestion since you were asking for one but don't expect it to be part of this PR.

I would still like to see the fix on SplitRequestTreeByType to be done in AllRequestsOfType instead though as it is currently made in the incorrect location

pawelbaran · 2021-02-19T09:05:51Z

I would still like to see the fix on SplitRequestTreeByType to be done in AllRequestsOfType instead though as it is currently made in the incorrect location

SplitRequestTreeByType requires a fix that is non-related to AllRequestsOfType, it is about handling of LogicalNotRequest a bit more explicitly than before 😞

…calNotRequest being null

pawelbaran · 2021-02-19T11:50:15Z

I have pushed the discussed fixes @adecler - not sure if this can be made look much better without major refactoring 🙈

adecler

Getting closer. Just a final change needed on SplitRequestTreeByType.cs

Data_Engine/Compute/SplitRequestTreeByType.cs

adecler · 2021-02-20T03:17:33Z

Data_Engine/Modify/SimplifyRequestTree.cs

+                                if (request is LogicalNotRequest)
+                                    ((LogicalNotRequest)request).Request = subSub[0];
                                else
-                                    subRequests.Insert(i, subSub[0]);
+                                {
+                                    subRequests.RemoveAt(i);
+
+                                    if (subSub[0].GetType() == request.GetType())
+                                        subRequests.InsertRange(i, ((ILogicalRequest)subSub[0]).IRequests());
+                                    else
+                                        subRequests.Insert(i, subSub[0]);
+                                }


I'll be honest, this whole method is confusing me a lot. I get the purpose but I just don't get why it is so complex. Since refactoring this method is not the purpose of this PR, I'll ignore it but I cannot guarantee that this part of the code is bug-free. So I'll have to trust you on this one (with the assumption that this will be reworked soon anyways as discussed).

Yeah, 100% agreed about everything above - happy to reiterate together with @alelom in the next milestone 👍

pawelbaran · 2021-02-22T17:15:12Z

@BHoMBot check compliance

bhombot-ci · 2021-02-22T17:15:14Z

@pawelbaran to confirm, check-code-compliance, check-documentation-compliance, check-project-compliance, check-branch-compliance, check-dataset-compliance, and, if applicable, check-copyright-compliance tasks are now queued.

pawelbaran · 2021-02-22T17:17:59Z

@BHoMBot check installer

bhombot-ci · 2021-02-22T17:18:00Z

@pawelbaran to confirm, check-installer task is now queued.

pawelbaran · 2021-02-22T17:23:02Z

/azp run BHoM_Engine.CheckInstaller

azure-pipelines · 2021-02-22T17:23:19Z

Azure Pipelines successfully started running 1 pipeline(s).

bhombot-ci · 2021-02-22T17:28:07Z

FAO: @FraserGreenroyd @pawelbaran is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is code-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. 1954410922

FraserGreenroyd · 2021-02-22T17:56:29Z

@BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. 1954410922

This is following a discussion with @pawelbaran and the agreement to raise #2332 to resolve this as it is outside the scope of this Pull Request which is already tackling an in depth issue.

bhombot-ci · 2021-02-22T17:56:31Z

@FraserGreenroyd I have now provided a passing check on reference 1954410922 as requested.

FraserGreenroyd · 2021-02-22T17:56:59Z

@BHoMBot check ready-to-merge

bhombot-ci · 2021-02-22T17:57:00Z

@FraserGreenroyd to confirm, the task for checking if this Pull Request is ready to merge is now queued.

FraserGreenroyd · 2021-02-22T17:57:13Z

@BHoMBot check serialisation

bhombot-ci · 2021-02-22T17:57:14Z

@FraserGreenroyd to confirm, check-serialisation task is now queued.

FraserGreenroyd · 2021-02-22T17:59:27Z

@BHoMBot check core

bhombot-ci · 2021-02-22T17:59:28Z

@FraserGreenroyd to confirm, check-core task is now queued.

adecler

I am now happy with the changes.
I guess what is left is to deal with the bot 😋

pawelbaran · 2021-02-23T09:17:24Z

@BHoMBot check ready-to-merge

bhombot-ci · 2021-02-23T09:17:25Z

@pawelbaran to confirm, the task for checking if this Pull Request is ready to merge is now queued.

pawelbaran added 3 commits February 15, 2021 22:21

AllRequestsOfType handling nulls nested in LogicalNotRequests fixed

a181e36

simplifying LogicalOrRequest/LogicalAndRequest with a single child ne…

5768e89

…sted inside LogicalNotRequest

SplitRequestTreeByType on LogicalNotRequest with a child IRequest of …

8d296ca

…the same type as splitting type fixed

pawelbaran added the type:bug Error or unexpected behaviour label Feb 15, 2021

pawelbaran requested a review from alelom February 15, 2021 21:30

pawelbaran self-assigned this Feb 15, 2021

pawelbaran requested review from adecler and epignatelli as code owners February 15, 2021 21:30

adecler requested changes Feb 18, 2021

View reviewed changes

pawelbaran added 2 commits February 19, 2021 12:46

Requests query fixed to return an empty list in case of child of Logi…

1e91f1e

…calNotRequest being null

code simplified

85513eb

pawelbaran requested a review from adecler February 19, 2021 11:50

adecler requested changes Feb 20, 2021

View reviewed changes

redundant call to SimplifyRequestTree removed

ad669be

pawelbaran requested a review from adecler February 22, 2021 16:12

adecler approved these changes Feb 23, 2021

View reviewed changes

FraserGreenroyd merged commit 861be54 into master Feb 23, 2021

FraserGreenroyd deleted the Data_Engine-#2310-NestedRequestIssues branch February 23, 2021 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data_Engine: Handling of nested request issues fixed #2311

Data_Engine: Handling of nested request issues fixed #2311

pawelbaran commented Feb 15, 2021 •

edited by FraserGreenroyd

Loading

adecler left a comment

pawelbaran commented Feb 18, 2021

adecler commented Feb 19, 2021

pawelbaran commented Feb 19, 2021

adecler commented Feb 19, 2021 •

edited

Loading

pawelbaran commented Feb 19, 2021

pawelbaran commented Feb 19, 2021

adecler left a comment

adecler Feb 20, 2021

pawelbaran Feb 22, 2021

pawelbaran commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

pawelbaran commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

pawelbaran commented Feb 22, 2021

azure-pipelines bot commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

FraserGreenroyd commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

FraserGreenroyd commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

FraserGreenroyd commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

FraserGreenroyd commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

adecler left a comment

pawelbaran commented Feb 23, 2021

bhombot-ci bot commented Feb 23, 2021

Data_Engine: Handling of nested request issues fixed #2311

Data_Engine: Handling of nested request issues fixed #2311

Conversation

pawelbaran commented Feb 15, 2021 • edited by FraserGreenroyd Loading

Issues addressed by this PR

Test files

Additional comments

adecler left a comment

Choose a reason for hiding this comment

pawelbaran commented Feb 18, 2021

adecler commented Feb 19, 2021

pawelbaran commented Feb 19, 2021

adecler commented Feb 19, 2021 • edited Loading

pawelbaran commented Feb 19, 2021

pawelbaran commented Feb 19, 2021

adecler left a comment

Choose a reason for hiding this comment

adecler Feb 20, 2021

Choose a reason for hiding this comment

pawelbaran Feb 22, 2021

Choose a reason for hiding this comment

pawelbaran commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

pawelbaran commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

pawelbaran commented Feb 22, 2021

azure-pipelines bot commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

FraserGreenroyd commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

FraserGreenroyd commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

FraserGreenroyd commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

FraserGreenroyd commented Feb 22, 2021

bhombot-ci bot commented Feb 22, 2021

adecler left a comment

Choose a reason for hiding this comment

pawelbaran commented Feb 23, 2021

bhombot-ci bot commented Feb 23, 2021

pawelbaran commented Feb 15, 2021 •

edited by FraserGreenroyd

Loading

adecler commented Feb 19, 2021 •

edited

Loading