Open takojunior opened 2 years ago
Hi Xin,
I believe you are correct. If negative values make sense for the application you are working on, then dropping the abs() should not be a problem. The zero values are assumed to indicate the non-presence of constraints, that's why lines 79-80 and 86-87 exist. For example, what if we define a lower gap constraint but no upper gap? This is currently done using a zero value, but should change if you are looking to consider zero as a valid lower or upper bound. Perhaps adding a NULL value (instead of zero) or another indication that only an upper or lower constraint exists should work. Dropping them without these changes is not recommended.
I wrote this code a couple of years ago so I'm not a 100% sure on everything. I suggest making changes and checking results on a dummy instance to be sure.
On Wed, Jun 29, 2022 at 5:08 PM Xin Wang @.***> wrote:
[External Email] [External Email]
Hi @aminhn https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_aminhn&d=DwMCaQ&c=sJ6xIWYx-zLMB3EPkvcnVg&r=LVYG5ZwyHgQ-qXJkzG2VEZbViMXR6NDEYtm7fvQVvJpSOCB49Vwyw3r9VAVQDJHo&m=UCNNlSRxq97ZoUOXerg2-wcYe5vEvV2SJPWu5mC3LUmx3sYGJHdE0nfGeAwq9SAA&s=J_ctSGtBlJ263MBmETmwoYV7CX2wsbek6iz7CJdbdmE&e=, I want to ask a question about checking the upper and lower gap constraints at this line https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_aminhn_MPP_blob_3d9761ebfb45fa64792060831ae44207bbd02c9d_src_build-5Fmdd.cpp-23L81&d=DwMCaQ&c=sJ6xIWYx-zLMB3EPkvcnVg&r=LVYG5ZwyHgQ-qXJkzG2VEZbViMXR6NDEYtm7fvQVvJpSOCB49Vwyw3r9VAVQDJHo&m=UCNNlSRxq97ZoUOXerg2-wcYe5vEvV2SJPWu5mC3LUmx3sYGJHdE0nfGeAwq9SAA&s=GswAsq3z_pqzm-JMHwxWtja966KA7gQbE_OavROBWDE&e=. I assume this is where we check the lower constraint for gap. If I want to use the original difference between two consecutive items' attributes, I wonder if it is safe to drop the abs(), then the calculated gap value can be positive or negative, and required upper/lower bound can also be positive/negative? If so, I wonder if the similar change would also make sense at line 46, 60 and 88 as well, where it is safe to drop abs()?
If the upper/lower bound can be positive/negative or zero, is it also safe to drop the lines 79-80 and 86-87?
Thanks very much!
— Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_aminhn_MPP_issues_3&d=DwMCaQ&c=sJ6xIWYx-zLMB3EPkvcnVg&r=LVYG5ZwyHgQ-qXJkzG2VEZbViMXR6NDEYtm7fvQVvJpSOCB49Vwyw3r9VAVQDJHo&m=UCNNlSRxq97ZoUOXerg2-wcYe5vEvV2SJPWu5mC3LUmx3sYGJHdE0nfGeAwq9SAA&s=kkWa34qwGmREIPe06Qu-CwFTht-e8Y4m0lcI_W8SBnk&e=, or unsubscribe https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AIVLLLBO7OEYGV6JYU3ENRLVRS26FANCNFSM52HD6WRQ&d=DwMCaQ&c=sJ6xIWYx-zLMB3EPkvcnVg&r=LVYG5ZwyHgQ-qXJkzG2VEZbViMXR6NDEYtm7fvQVvJpSOCB49Vwyw3r9VAVQDJHo&m=UCNNlSRxq97ZoUOXerg2-wcYe5vEvV2SJPWu5mC3LUmx3sYGJHdE0nfGeAwq9SAA&s=51_e5W5ImNSnots2wCwZKnd3OWfLcXth9MM2dRzur68&e= . You are receiving this because you were mentioned.Message ID: @.***>
--
Amin Hosseininasab Assistant Professor of Marketing Department of Marketing Warrington College of Business University of Florida https://warrington.ufl.edu/directory/person/12519/
Hi Xin, I believe you are correct. If negative values make sense for the application you are working on, then dropping the abs() should not be a problem. The zero values are assumed to indicate the non-presence of constraints, that's why lines 79-80 and 86-87 exist. For example, what if we define a lower gap constraint but no upper gap? This is currently done using a zero value, but should change if you are looking to consider zero as a valid lower or upper bound. Perhaps adding a NULL value (instead of zero) or another indication that only an upper or lower constraint exists should work. Dropping them without these changes is not recommended. I wrote this code a couple of years ago so I'm not a 100% sure on everything. I suggest making changes and checking results on a dummy instance to be sure.
Thanks a lot Aminhn! I would check the possible changes accordingly based on your suggestions and test on dummy data samples.
I remember that we discovered a minor bug in the Gap Constraint a while back and Amin pushed a fix for it, albeit in the original repo and not in Seq2Pat.
@takojunior Is this issue related to that bug? If so, how does that compare to Amin's fix --you find his bug fix commit in the other repo and see what changed in the Gap Constraint backend
I remember that we discovered a minor bug in the Gap Constraint a while back and Amin pushed a fix for it, albeit in the original repo and not in Seq2Pat.
@takojunior Is this issue related to that bug? If so, how does that compare to Amin's fix --you find his bug fix commit in the other repo and see what changed in the Gap Constraint backend
Hi @skadio, I think this is related to an earlier fix at this line. @aminhn please correct me if this is not correct. I know the fix was made two years ago. I find this commit in the history. So in the fix, abs() is added to the statement when we check conditions.
In my proposed change, I want to not use abs in these conditions, then changes could be needed at lines 46, 60, 81 and 88, where we can drop abs() which is currently used.
Hi @aminhn, I want to ask a question about checking the upper and lower gap constraints at this line. I assume this is where we check the lower constraint for gap. If I want to use the original difference between two consecutive items' attributes, I wonder if it is safe to drop the abs(), then the calculated gap value can be positive or negative, and required upper/lower bound can also be positive/negative? If so, I wonder if the similar change would also make sense at line 46, 60 and 88 as well, where it is safe to drop abs()?
If the upper/lower bound can be positive/negative or zero, is it also safe to drop the lines 79-80 and 86-87?
Thanks very much!