Added documentation for PyBTLS interface #49

Rizqiansyah · 2023-09-05T02:24:09Z

Hi all,

This pull request adds the (severely needed) documentation for the interface modules, as requested by @CaiZhou2.

ccaprani

Lots of good functions here, but there seems to be a lot of replication of the internals of the C++ code which should be unnecessary. Each class/function should have a clear need in an identified workflow.
Some naming is counter to python conventions.
Use of files...a clear use-case workflow is needed since when run through python most outputs should be just in memory perhaps?

ccaprani · 2023-09-05T02:34:01Z

py/PyBTLS/py/bridge/bridge.py

+        """
+        Private method to get influence lines.
+        IL computed using PyCBA, then compressed using the maxdistance algorithm.
+        """
        # IL to be compressed
        ils = cba.InfluenceLines(self.L, np.ones(len(self.L)), self.R)


This means pycba is a dependency for PyBTLS. It's not a good idea to have too many dependencies. Surely a user could work out their own IL outside PyBTLS and just pass it in; it would avoid a hard dependency.

I'll try to add a method (in a separate PR) to manually add IL. Perhaps we can save this method, by checking if PyCBA is installed, then users can use this method. Else throw an exception?

ccaprani · 2023-09-05T02:34:24Z

py/PyBTLS/py/bridge/bridge.py

 class Bridge:
+    """
+    Python side Bridge Class.


Is this just a wrapper of the PyBTLS class?

It doesn't directly interface with the Bridge class in c++. It simply wraps the txt/csv file creation.

ccaprani · 2023-09-05T02:35:11Z

py/PyBTLS/py/bridge/bridge.py

        with open(path, "w") as f:
            f.write(self._create_il_txt())

    def write_as_txt(self, il_path="IL.txt", bridge_path="bridge.txt"):
+        """
+        Method to write both the bridge and influence line text file for BTLS to run.
+        """
        self.write_bridge_as_txt(bridge_path)
        self.write_il_as_txt(il_path)


Once something is defined in memory, we should be able to avoid needing to write to HDD.

ccaprani · 2023-09-05T02:36:27Z

py/PyBTLS/py/bridge/flow.py

-    # via DataFrame (base class has this covered)
-    # or via file, which needs to be redefined to not use the _from_txt_to_dataframe_kwargs() method
-    # And block the _from_txt_to_dataframe_kwargs() method from working
-
    def _from_txt_to_dataframe_kwargs(self, txt, *args, **kwargs):


Is there a required format of the dataframe and/or textfile?

The default Lane Flow files are all in csv, which this class can load.

It's only blocking the _from_txt_to_dataframe_kwargs as this method strictly initializes using a txt file, loading it as string, parsing it line by line, and then create a dataframe. It's too complicated this way; the pandas csv loader can do the job just fine.

Also just a quick note the reason _from_txt_to_dataframe_kwargs is there is because it's inherited from DFBased class.

ccaprani · 2023-09-05T02:36:59Z

py/PyBTLS/py/bridge/flow.py

-
-"""
-
-
 class BridgeFlow(_ListLike):


I think this is an internal class of PyBTLS. Not sure why a user would need access to it?

Yes, I'm still not happy with how the lane flows are defined in python right now. Hopefully Ziyi and I can reach a better workflow soon. Ziyi will let you know.

ccaprani · 2023-09-05T02:37:42Z

py/PyBTLS/py/output/output.py

@@ -8,6 +8,11 @@


 class _BtlsWrapper_DefaultOutputs:


Seems unnecessary. Also naming convention - stick to PEP.

ccaprani · 2023-09-05T02:38:12Z

py/PyBTLS/py/output/output.py

@@ -18,6 +23,17 @@ def __str__(self):


 class CumulativeStatistics(_DfBased):


Is this for reading in a file output from PyBTLS C++?

Right now it's only reading from output txt/csv file.

ccaprani · 2023-09-05T02:39:28Z

py/PyBTLS/py/output/output_reader.py

+This file provide functions to import BTLS ouput for a given bridge span.
+These functions are for convenience; 
+the output file names from BTLS are sometimes inconsistent.
+To import directly from a know file path, please use the appropriate Classes directly.


I guess in normal python useage, we won't need to read from files; but probably useful to have these anyway. Should "OutputReader" not be a class perhaps?

ccaprani · 2023-09-05T02:40:03Z

py/PyBTLS/py/traffic/PyBtlsTraffic.py

@@ -11,6 +11,11 @@


 class BaseTrafficFile:


This seems an internal class to the C++ code and not for users to interface with?

ccaprani · 2023-09-05T02:40:24Z

py/PyBTLS/py/utils_interface/_helper_class.py

-        Not wholly supported after initialisation, though user can manipulate the existing DataFrame like regular
-"""
-
-
 class _DfBased(pd.DataFrame):


Stick to naming conventions.

Rizqiansyah · 2023-09-05T03:20:32Z

Lots of good functions here, but there seems to be a lot of replication of the internals of the C++ code which should be unnecessary. Each class/function should have a clear need in an identified workflow. Some naming is counter to python conventions. Use of files...a clear use-case workflow is needed since when run through python most outputs should be just in memory perhaps?

Thanks Colin. We (mostly Ziyi) is trying to resolve the replicated functions. This PR is just the documentations for the wrapper of the old method writing to txt/csv files, but in more python friendly ways (e.g., using DataFrame, etc.). There's an example workflow in the tests/PyBTLS_interface folder. I haven't changed any functions/classes since that example was written last year. This PR only added documentation.

On the outputs, I find it useful to have the output written to HDD, as I often run multiple instances of BTLS, then merge the outputs (very easily using the classes written here using pandas.concat). I don't think it will be trivial to code for this use case if outputs are only in memory.

I also changed the class names to follow the convention better, should be in the next commit soon.

Added documentation for PyBTLS interface

62ee7d3

ccaprani reviewed Sep 5, 2023

View reviewed changes

Rizqiansyah and others added 2 commits September 5, 2023 13:35

Changed class names, removed preceeding underscore

a6e97db

define data in RAM

14d4665

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added documentation for PyBTLS interface #49

Added documentation for PyBTLS interface #49

Rizqiansyah commented Sep 5, 2023

ccaprani left a comment

ccaprani Sep 5, 2023

Rizqiansyah Sep 5, 2023

ccaprani Sep 5, 2023

Rizqiansyah Sep 5, 2023 •

edited

Loading

ccaprani Sep 5, 2023

ccaprani Sep 5, 2023

Rizqiansyah Sep 5, 2023

ccaprani Sep 5, 2023

Rizqiansyah Sep 5, 2023

ccaprani Sep 5, 2023

ccaprani Sep 5, 2023

Rizqiansyah Sep 5, 2023

ccaprani Sep 5, 2023

ccaprani Sep 5, 2023

ccaprani Sep 5, 2023

Rizqiansyah commented Sep 5, 2023

		@@ -18,6 +23,17 @@ def __str__(self):


		class CumulativeStatistics(_DfBased):

Added documentation for PyBTLS interface #49

Are you sure you want to change the base?

Added documentation for PyBTLS interface #49

Conversation

Rizqiansyah commented Sep 5, 2023

ccaprani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rizqiansyah Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rizqiansyah commented Sep 5, 2023

Rizqiansyah Sep 5, 2023 •

edited

Loading